Fix Content-Length byte/char mismatch in the server-mode TCP transport - #10297
Conversation
TcpMessageHandler declares Content-Length in UTF-8 bytes on the write path but consumed that number as decoded characters on the read path (ReadBlockAsync over a StreamReader). For pure ASCII the two counts coincide, so nothing broke. For any multi-byte UTF-8 content - a localized test name, an exception message, or captured standard output in German, Japanese, Czech, Cyrillic, or an emoji - the reader under-reads the frame, leaves its tail in the stream, and the framing desynchronizes permanently: the leftover bytes are parsed as the next frame's headers. The symptom therefore surfaces on the second frame, not the first. Both compilation branches were affected: the #if NETCOREAPP path via ReadBlockAsync(Memory<char>) and the #else (Jsonite, net462/netstandard2.0) path via ReadBlockAsync(char[], ...). The read path now consumes exactly Content-Length bytes and UTF-8-decodes them, symmetric with the write path. Because StreamReader would have buffered part of the body while reading the headers, the headers are read at the byte level too: header and body share one buffering boundary, so nothing can be stranded on the other side of it. The previous StreamReader had byte-order-mark detection on by default, so a leading UTF-8 preamble is still skipped for peers that write their headers through a preamble-emitting encoder. This file is shared with the live MTP server, not only with the new source-only client package, so the fix applies to the shipping server transport as well. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 365edd84-6344-4ce4-9048-2c1ed1b7d939
Swaps the interim feed pack from the plain 2.4.0-dev build output to the coordinator's canonical 2.4.0-dev.utf8fix1 drop of microsoft/testfx#10297. Byte-equivalent content: all 184 contentFiles are identical between the two packs, including TcpMessageHandler.cs with both ReadExactlyAsync and the TrimPreamble BOM tolerance. Only the version metadata differs. The rename is the point. While the package is served from a committed local folder, NuGet caches by version, so a plain 2.4.0-dev risks silently resolving a stale cache entry from an earlier drop of the same name. The unique suffix makes that impossible, matching the convention the branch already used for 2.4.0-dev.numberfix. Re-verified from a cleared package cache: CrossPlatEngine clean on all three TFMs, MTP unit tests 140/140, MtpUnderVstestTests 16/16. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f
Amaury Levé (Evangelink)
left a comment
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
Summary
Reviewed in depth and validated locally — this is a real, correctly-diagnosed bug and the fix is sound. Approving.
What I verified (not just read)
I checked out the head commit into a worktree, built both TFM legs, and ran the suites:
| Check | Result |
|---|---|
Microsoft.Testing.Platform.ServerClient.UnitTests (net8.0) |
28/28 pass |
Microsoft.Testing.Platform.ServerClient.UnitTests (net462) |
28/28 pass |
Microsoft.Testing.Platform.UnitTests (net8.0, incl. ServerTests) |
1440/1440 pass |
Microsoft.Testing.Platform.UnitTests (net462) |
1416/1416 pass |
| Build both legs | clean, 0 warnings |
The regression tests genuinely fail without the fix. I reverted only TcpMessageHandler.cs to its parent revision, rebuilt, and re-ran — ReadAsync_RawUtf8FramesFromPeer_DoesNotDesynchronizeSubsequentFrames and ReadAsync_LeadingByteOrderMark_IsSkipped both fail on net8.0 with exactly the 'C' is invalid after a single JSON value reader exception the description quotes (the C of the next frame's Content-Length). That is a properly failing-before/passing-after regression test, not a test written to match the new implementation.
The bug is confirmed present on main, not just on the stacked base branch: main's WriteRequestAsync declares Encoding.UTF8.GetByteCount(messageStr) while ReadAsync consumes that count through StreamReader.ReadBlockAsync (characters) on both compilation branches. So this really is a shipping server-transport defect, and the framing desynchronizes permanently once any multi-byte body goes over the wire.
Correctness review of the new read path
The reasoning about StreamReader is right and worth keeping in the comment: reading headers through a StreamReader and the body from BaseStream would have been strictly worse, because the reader's internal buffer would already have swallowed part of the body. Collapsing both onto one byte-level buffer is the correct shape.
Specific points I checked and found sound:
FillReadBufferAsyncis only ever called when_readBufferOffset == _readBufferCount, so the unconditional_readBufferOffset = 0cannot discard unconsumed bytes.- Offset/count are assigned only after the
await, so a throwing/cancelled read leaves the buffer state intact. GetCharCount/GetCharsuse the sameEncoding.UTF8replacement fallback, so the counts cannot disagree on malformed input.- Both pooled buffers are returned on every exit path, including the mid-frame-disconnect
return null. Dispose()swapping_reader.Dispose()for_readStream.Dispose()is behavior-preserving:StreamReaderowned the stream (leaveOpen: false), and both real call sites (MessageHandlerFactoryandMtpServerProcess) pass the sameNetworkStreamfor both directions.- Cancellation semantics are genuinely unchanged: the removed
NET7_0_OR_GREATER/NET6_0_OR_GREATERladder existed only forStreamReader.ReadLineAsync, which forwards the token to the sameStream.ReadAsyncthe new code calls directly.
To back the parts the PR's own tests do not reach, I wrote six throwaway probes against the fixed handler — a 20 KB body spanning many FillReadBufferAsync calls, a 5 KB header line forcing several lineBuffer doublings, a frame dribbled one byte at a time so header and body straddle refills at arbitrary offsets, bare-LF headers, and a peer that disconnects mid-body. All six pass: the buffer-boundary logic is correct. (Deleted afterwards — see the coverage note inline.)
Findings
Nothing blocking. Three non-blocking notes inline: a per-frame allocation on the hot read path, a test-coverage gap for the multi-iteration buffer paths, and a pre-existing unbounded Content-Length that this change makes strictly better rather than worse.
One process question
The src/ half of this fix is independent of #10085 and applies to main as-is, but the PR is stacked on nohwnd-mtp-client-source-package because the regression test lives in the new ServerClient.UnitTests project. That couples a shipping-server bug fix to the packaging work. Worth deciding explicitly whether you want the transport fix to be able to merge to main on its own (e.g. with the test placed in Microsoft.Testing.Platform.UnitTests instead) — entirely your call, and not a reason to hold this up if #10085 is landing shortly.
…ouple from microsoft#10085 Follow-up to the review on microsoft#10297. Three non-blocking findings plus the process question, all addressed. Reuse the header line buffer (review: per-frame allocation) ReadHeaderLineAsync allocated a fresh 256-byte array per header line, roughly three per frame, on the hottest read path -- server mode emits a testing/testUpdates/tests notification per test. The buffer is now an instance field grown in place. This is safe for the same reason _readBufferOffset and _readBufferCount already are: reads are single-threaded by construction, driven by exactly one read loop. Document the unbounded Content-Length as a conscious decision (review: observation) commandSize is still trusted unbounded. That is deliberate: server mode is a loopback channel to a test host this process launched, and legitimate bodies are unbounded in principle, so a cap would be a guess that risks rejecting valid traffic. Recorded in a comment rather than changed, and noted that the byte-based read more than halves the previous worst case, which rented commandSize chars. Cover the multi-iteration buffer paths (review: coverage gap) The previous tests all used frames under ReadBufferSize and header lines under HeaderLineInitialCapacity, so the two loops this change introduced only ever ran a single iteration. Three tests added: - a body far larger than the read buffer, so ReadExactlyAsync refills several times and the body spans buffer boundaries; - a header line far longer than the initial header buffer, so the growth path runs through several doublings; - a peer that announces a body then disconnects mid-way, which must surface as ReadAsync returning null rather than hanging or throwing. The first and third fail against the pre-fix transport; the second guards a path this change introduced. Decouple the fix from the source-package work (review: process question) The src/ half applies to main as-is, but the tests lived in the new ServerClient.UnitTests project, coupling a shipping-server bug fix to the packaging change. They now live in Microsoft.Testing.Platform.UnitTests, merged into the existing TcpMessageHandlerTests. That project targets net462 alongside net8.0/net9.0, so both the #if NETCOREAPP (System.Text.Json) and #else (Jsonite) compilation branches are still covered, and this PR no longer depends on microsoft#10085 for anything. Assertions were reworked to compare the JSON-RPC method rather than a params payload: params binding requires a registered method, so the previous payload-based assertions were not portable to a transport-level test outside the ServerClient harness. The non-ASCII content moved into the method name, which both formatters always bind, so the assertions still compare every multi-byte character rather than merely checking that a frame arrived. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 365edd84-6344-4ce4-9048-2c1ed1b7d939
…rage On .NET the System.Text.Json formatter escapes non-ASCII to \uXXXX, so that test's body reaches the wire as pure ASCII and it degrades to an ASCII round trip there. The docstring only hinted at this; state plainly which tests do carry the cross-TFM coverage so a future reader does not over-trust it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 365edd84-6344-4ce4-9048-2c1ed1b7d939
|
Thanks for the depth here — reverting All three inline findings are addressed in Changes since your reviewPer-frame allocation — Unbounded Coverage gaps — three tests added, covering the The process question — the fix is now genuinely independent of #10085You were right that this was worth deciding explicitly, and acting on it turned out to be cheap. The tests have moved from I verified the decoupling rather than assuming it. Rebasing all three commits onto current
So the transport fix can merge to One thing worth knowing either way: the fix is validated end to end outside this repo. A locally-built pack of the source package was consumed by the vstest retarget work, where CrossPlatEngine builds clean on net462/netstandard2.0/net8.0 and the MTP suites pass 140/140 and 16/16 — including the raw-UTF-8 peer traffic that is precisely what exposes this bug. Test results after the changes
The platform suites are up 7 (4 tests moved in from One coverage caveat I made explicitWhile re-reviewing the committed change I confirmed something the docstring had only hinted at, and stated it plainly in The cross-TFM coverage comes from |
Replaces the interim local-feed pack with a build of microsoft/testfx#10297, which stacks the Content-Length byte/char fix onto microsoft#10085. The transport now reads exactly Content-Length bytes and UTF-8-decodes them, symmetric with the write path, and reads the headers through the same byte-level buffer so no StreamReader can buffer part of the body across the boundary. That drop also carries microsoft#10085's ServerRequestHandler signature change (the result is now constrained to a serializable dictionary), so FakeMtpServerClient is updated to match. Verification on this drop: - CrossPlatEngine builds clean on net462, netstandard2.0 and net8.0. - MTP unit tests 140/140 (70 per axis, net11.0 and net481). - MtpUnderVstestTests 16/16 on both console axes. Note the 16/16: the two /logger:trx failures reported against the earlier drop do not reproduce here, so they look like a local deployment issue rather than anything in the retarget. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f
Amaury Levé (Evangelink)
left a comment
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
New review pass
Re-reviewed after a12f08a0 / 58bba249. The diagnosis and the fix are correct, and I verified it rather than reading it. Nothing below blocks; the first two are the ones I'd actually act on.
What I ran
Built Microsoft.Testing.Platform.UnitTests (net8.0/net9.0/net462) from the head commit:
| Check | Result |
|---|---|
TcpMessageHandlerTests — net8.0 |
8/8 pass |
TcpMessageHandlerTests — net462 |
8/8 pass |
Full Microsoft.Testing.Platform.UnitTests — net8.0 |
1932/1932 pass |
Full Microsoft.Testing.Platform.UnitTests — net462 |
1891/1891 pass |
Then I reverted only TcpMessageHandler.cs to dbb946d4~1, rebuilt, and re-ran, to confirm the tests are genuine failing-before regression tests rather than fitted to the new implementation:
failed ReadAsync_RawUtf8FramesFromPeer_DoesNotDesynchronizeSubsequentFrames
System.Text.Json.JsonReaderException: 'C' is invalid after a single JSON value.
failed ReadAsync_LeadingByteOrderMark_IsSkipped
System.Text.Json.JsonReaderException: 'C' is invalid after a single JSON value.
failed ReadAsync_BodyLargerThanReadBuffer_SpansMultipleRefillsWithoutDesynchronizing
Timed out reading a frame; the transport is most likely desynchronized.
failed ReadAsync_PeerDisconnectsMidBody_ReturnsNull
Timed out reading a frame; the transport is most likely desynchronized.
total: 8, failed: 4, succeeded: 4
4 of the 8 fail pre-fix on net8.0 alone. Claim confirmed.
Reader correctness
Points I specifically convinced myself of, since this is framing code where a subtle error desynchronizes a live connection:
- Headers and body share one
_readBufferwith one_readBufferOffset/_readBufferCountpair, and bothReadHeaderLineAsyncandReadExactlyAsyncdrain it before refilling. That's the invariant the whole fix rests on and it holds — no bytes can be stranded on the wrong side of a buffering boundary, which is exactly the trap that made the naive "headers viaStreamReader, body viaBaseStream" fix wrong. - Header lines are accumulated as bytes and decoded once at the terminator, so a multi-byte character — or the BOM itself — split across a refill is handled correctly. Easy to get wrong with a per-chunk decode; this doesn't.
TcpMessageHandleris the onlyIMessageHandlerimplementation in the tree, so there's no sibling transport carrying the same defect.- The token reaching
ReadAsyncis_testApplicationCancellationTokenSource.CancellationToken(PassiveNode), i.e. connection-lifetime. So the net462WithCancellationAsyncpattern abandoning a read that still owns_readBufferis a teardown-only concern, not a mid-connection corruption risk. Worth having checked given the buffer is now shared instance state rather than a per-call local. Disposeswapping_reader.Dispose()for_readStream.Dispose()is equivalent —StreamReader.Disposedisposed the underlying stream anyway.
1. The .NET path now transcodes bytes → chars → bytes
Json.Deserialize<T>(ReadOnlyMemory<char> utf8Json) calls JsonDocument.Parse(ReadOnlyMemory<char>), which immediately transcodes back to UTF-8 into its own pooled array:
// System.Text.Json
public static JsonDocument Parse(ReadOnlyMemory<char> json, JsonDocumentOptions options = default)
{
ReadOnlySpan<char> jsonChars = json.Span;
int expectedByteCount = JsonReaderHelper.GetUtf8ByteCount(jsonChars);
byte[] utf8Bytes = ArrayPool<byte>.Shared.Rent(expectedByteCount);
...So on #if NETCOREAPP the new code does GetCharCount + Rent<char> + GetChars, and STJ then undoes all three — while the raw UTF-8 bytes we started from were already sitting in bodyBuffer. JsonDocument.Parse(ReadOnlyMemory<byte> utf8Json) is the overload that wants exactly what we have.
An IMessageFormatter.Deserialize<T>(ReadOnlyMemory<byte>) overload (NETCOREAPP only — net462/Jsonite keeps Encoding.UTF8.GetString) removes one transcode and one pooled rent per frame on the hottest read path, which is the same path this PR is otherwise careful to keep allocation-free.
Worth doing partly for a non-performance reason: Deserialize<T>(ReadOnlyMemory<char> **utf8Json**) and MessageFormatter.Deserialize<T>(ReadOnlyMemory<char> **serializedUtf8Content**) are a char-typed parameter with a utf8-shaped name. That naming is the byte/char ambiguity this PR exists to fix, still present one layer down. Fine as a follow-up, but it belongs on the record next to this fix rather than rediscovered later.
2. Retarget to main
Base is nohwnd-mtp-client-source-package (#10085), still open. This fixes the shipping server-mode transport, and the description already establishes it rebases onto main cleanly with the tests landing in a project that exists there. I'd retarget now rather than leave a live-transport fix gated on an unrelated packaging PR.
3. Negative Content-Length crashes the read loop (verified)
The new comment says commandSize is deliberately trusted unbounded, and I agree with that reasoning for large values. But negative values aren't just unbounded — they throw. I probed it:
Content-Length: -5
PROBE-RESULT: threw System.ArgumentOutOfRangeException:
minimumLength ('-5') must be a non-negative value. (Parameter 'minimumLength')
int.TryParse happily yields -5, the is -1 guard doesn't catch it, and ArrayPool<byte>.Shared.Rent(-5) throws past the SocketException/IOException filter — so a malformed header takes down the read loop with an unrelated exception type instead of the graceful null disconnect every other malformed-frame path produces. (net462 gets OverflowException from new byte[-5].)
Pre-existing, and I wouldn't ask for it normally. But the PR now carries a comment asserting this input was consciously reasoned about, and if (commandSize < 0) { return null; } is one line that makes the comment true.
4. The trust-boundary argument is weaker for _headerLineBuffer than for the body
The comment justifies unbounded header-line growth by pointing at Content-Length's trust boundary. The two aren't equivalent: bodies are legitimately unbounded (a stack trace, captured stdout), which is what makes refusing to cap them correct. Header lines have no legitimate large case, and a peer that simply never sends \n doubles the buffer forever. If a cap is ever wanted, headers are both cheaper and more defensible than the body — worth saying so rather than grouping them.
5. Undocumented line-terminator delta
StreamReader.ReadLineAsync treated a bare \r as a terminator; ReadHeaderLineAsync breaks only on \n. Protocol-irrelevant, and arguably stricter-is-better — but the comment reads as if it enumerates the cases:
// Tolerate both CRLF and a bare LF.Adding "a bare CR is not a terminator, unlike StreamReader" closes it.
Test nits
ConnectedHandlers.CreateAsyncleakslistenerandclientSocketifConnectAsync/AcceptTcpClientAsyncthrows.WriteRawFrameuses blockingstream.Write, and every frame is written before any read starts.ReadAsync_BodyLargerThanReadBuffer_...pushes ~27 KB that way and relies on socket buffers absorbing it. Fine today, but the margin is invisible in the test, so growing the payload later turns a clear assertion failure into a CI hang. Starting the read first — or a comment naming the constraint — makes the dependency explicit.ReadWithTimeoutAsyncleaves an uncancelled 30 sTask.Delayper call.ReadAsync_LeadingByteOrderMark_IsSkippedcarriesNonAsciiMethod, so pre-fix it fails for the byte/char reason rather than isolating BOM handling. I checked it does still catch a BOM-only regression (the\uFEFFprefix makes the first header unparseable →contentSizestays -1 →ReadAsyncreturns null → the cast yields null → NRE on.Method), so the coverage is real. The failure mode just won't point at the BOM.
Good change — the byte-level rewrite is the right shape rather than a patch over ReadBlockAsync, and the pre-fix failure evidence holds up under independent reproduction. My only real asks are the retarget (#2) and, if you want it in scope, the one-line negative guard (#3).
cc44d19
into
microsoft:nohwnd-mtp-client-source-package
…ient.Sources (#16300) * Retarget the MTP client onto Microsoft.Testing.Platform.ServerClient.Source testfx now ships vstest's MTP server-mode JSON-RPC client as a source-only package built from the MTP server's own protocol and serialization source, so the wire format cannot drift from the server. Delete vstest's transport core (MtpServerConnection, MtpJson, MtpConstants, MtpClientHelpers) and retarget the glue onto the package's IMtpServerClient: launch via MtpServerClient.Launch, drive Initialize/Discover/Run/Exit, read node updates from the TestNodesUpdated event with typed MtpTestNodeUpdate accessors, and bridge EqtTrace through DelegateMtpClientLogger. MtpClientOptionsFactory centralizes option construction and log-level mapping. The package is a compile-time source dependency (PrivateAssets=all), so no runtime dependency and no public API are added. Blocked on testfx publishing the package (microsoft/testfx#10085); references an interim local feed, so CI cannot restore it yet. * Commit the interim local MTP client feed so restore works everywhere NuGet.config pointed local-mtp at the absolute path Q:\q\local-mtp-feed, which is machine-local and does not exist in CI, so restore failed with an incorrect path. Move the feed under the repo at eng/local-mtp-feed, point NuGet.config at that repo-relative path, and commit the package into the feed. .gitignore keeps ignoring *.nupkg but adds a negation for eng/local-mtp-feed/*.nupkg so the feed package is tracked. The package is the fresh Design-A drop of Microsoft.Testing.Platform.ServerClient.Source 2.4.0-dev, which builds CrossPlatEngine clean on net462, netstandard2.0, and net8.0 (0 errors, 0 warnings) with the retargeted glue. Interim only; remove the feed once microsoft/testfx#10085 ships the package to a public feed. 🤖 * Order remote NuGet feeds before the interim local feed in test asset restore The acceptance tests restore the TestAssets solution, which transitively restores product projects like CrossPlatEngine that now reference the interim local-mtp feed. Passing that local-folder feed to dotnet restore alongside the remote https feeds triggered two NuGet quirks, both surfacing as NU1301: a relative --source path is rooted at each restored project's directory, and a local-folder source placed before the remote sources mis-normalizes the https URLs into per-project relative paths. Resolve relative local-folder sources to absolute paths and emit the remote sources first so all local-folder sources come last; remote feeds keep their configured order. Only needed while the MTP client package lives on the interim local feed, and harmless once testfx#10085 ships it to a public feed. 🤖 * Key MTP environment variable dictionary case-insensitively on Windows Both places that collect environment variables for the MTP application launch now share one comparer: case-insensitive on Windows, case-sensitive elsewhere. Before, the runsettings path used that comparer but the data-collector-only path used a plain ordinal dictionary, so a run with no runsettings variables but with data-collector variables lost the case-folding the classic testhost path applied on Windows. The package options dictionary is ordinal, so deduping here preserves the classic Windows semantics before the values reach it. 🤖 * Consume official B-fixed MTP client source drop (testfx#10085) Replaces the interim 2.4.0-dev pack with the official drop that fixes the STJ number-decode bug: untyped JSON numbers were hard-cast to Int32, so node bags carrying doubles (durations) or longs (timestamps) threw FormatException and faulted the MTP read loop on the net8 client. The fix decodes numbers generically (ReadNumber: TryGetInt32 -> TryGetInt64 -> TryGetUInt64 -> double). Pinned to the unique version 2.4.0-dev.20260721161520 to avoid NuGet same-version cache collisions while the package is served from the committed local feed. MtpUnderVstestTests: net11.0 (STJ) axis now 7/7 (was 0/7); net481 (Jsonite) axis 5/7. The 2 remaining failures are a pre-existing net462 TRX-logger load issue that also breaks classic non-MTP trx tests, unrelated to this retarget. 🤖 * Align interim MTP client pin to the coordinator's canonical numberfix drop Swaps the interim feed pack and pin from the timestamped unique 2.4.0-dev.20260721161520 to the coordinator's canonical uniquely-named drop 2.4.0-dev.numberfix (MD5 FC7F7A9F68EF482718B61DC9DA5F38B4). Byte-equivalent fixed content -- the packed net8 Json.Deserializers.cs decodes untyped JSON numbers via ReadNumber at both sinks (L55/L97, helper L344), same as the prior drop -- this only adopts the stable canonical interim identity the package owner is standardizing on across consumers. Validation unchanged: MtpUnderVstestTests net11.0 (STJ) axis 7/7, full suite 12/14 (the 2 remaining failures are the pre-existing net462 TRX-logger load issue, unrelated to this retarget). 🤖 * Add MTP converter/options unit tests and fix numeric and trait coercion The retarget onto Microsoft.Testing.Platform.ServerClient.Source left the MTP glue with no unit coverage at all - the only tests were the end-to-end MtpUnderVstestTests. The conversion code is now pure and dependency-free, so cover it directly. Add MtpTestNodeConverterTests and MtpClientOptionsFactoryTests (55 tests) covering the normalized-Node contract, per-formatter number boxing, outcome mapping, the action-node filter, vstest bridge properties, standard output/error, traits, duration and log-level mapping. Three fixes fall out of writing them: - TryGetRawInt wrapped out-of-range values with unchecked((int)l), turning a bad line number into a plausible-looking wrong answer. Range-check instead so the property stays at its visibly-unset default. - AddTraits collapsed every non-string trait value to an empty string. The two formatters box JSON scalars differently, so a numeric or boolean trait was silently dropped on one formatter and kept on the other. Format invariantly. - MtpClientOptionsFactory re-read VSTEST_CONNECTION_TIMEOUT and hardcoded the 90-second default instead of calling EnvironmentHelper.GetConnectionTimeout, which seven other vstest call sites already use and which also traces the override. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Fix MTP client shutdown and fail loudly on a missing node uid Retargeting onto the source package changed exit from a fire-and-forget notification into an awaited request/response call, which introduced two regressions: - Exit was awaited on the run's own cancellation token. Cancelling or aborting a run is exactly when that token is already cancelled, so ExitAsync threw immediately and the graceful shutdown handshake was skipped in the one case it matters most. - The await was unbounded, so a test application that never acknowledges exit would hang discovery or execution indefinitely. The notification it replaced could not block at all. Route both proxy managers through MtpServerClientFactory: TryExit runs on its own bounded token, swallows failures (the caller disposes the client next, which tears the process down regardless), and is called from a finally block so a failed or cancelled run still shuts the application down. The factory also exposes a replaceable Launch delegate so the managers can be driven against a fake server in unit tests; production always uses MtpServerClient.Launch. Separately, BuildUids substituted FullyQualifiedName when a TestCase carried no MTP.TestNode.Uid. The server projects node.Uid alone when building a run filter and never reads any other field, so that substitution produced a filter matching nothing: the run reported success having executed zero of the tests the user selected, with no error anywhere. Throw instead, with a comment explaining why no fallback is correct. Adds 15 tests covering the shutdown paths, the uid filter, and both manager flows against a fake MTP server. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Add non-ASCII MTP acceptance coverage for UTF-8 frame length MTP frames declare Content-Length in UTF-8 bytes, but the transport shipped by Microsoft.Testing.Platform.ServerClient.Source reads that number of characters: it rents a char buffer of Content-Length and calls StreamReader.ReadBlockAsync. For any frame carrying multi-byte UTF-8 the two disagree, so the reader under-reads and leaves the body's tail to be parsed as the next frame's headers - the connection desynchronizes from the following message onward. vstest's deleted MtpServerConnection was byte-correct here (it read Content-Length bytes into a byte[] and then UTF-8-decoded), so the retarget is a regression, not an inherited defect. Client-to-server traffic is ASCII in practice, which is why it has not surfaced; node updates flow the other way and carry user-authored test names. Give MtpMSTestProject a test whose display name mixes German umlauts (2 bytes each), Japanese (3 bytes each) and an emoji (4 bytes, 2 chars), and mirror it in MtpPureProject. Because the corruption lands on the message *after* the offending one, its mere presence makes the whole run fail rather than just that test, so every existing MTP scenario now exercises the transport with multi-byte content. Adds a dedicated test asserting the name survives into the TRX. These fail until the fix lands upstream in testfx. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Narrow the non-ASCII MTP test name to the BMP and fix the collector count Running the acceptance test revealed two things worth recording. First, an end-to-end MTP run cannot reproduce the Content-Length byte-vs-char framing bug: the .NET MTP server serializes with System.Text.Json, whose default encoder escapes every non-ASCII character to \\uXXXX, so the bytes on the wire are ASCII and the byte count coincidentally equals the character count. The framing bug is real but has to be proved at the unit level against the transport directly, which is what the companion testfx change does. This test is therefore a name-integrity guard, and its comments now say so rather than overclaiming. Second, the emoji originally in the name exposed a separate defect: astral-plane characters are escaped by System.Text.Json as a surrogate pair and arrive in the TRX as the literal text \\ud83c\\udf89 instead of the character. BMP characters decode correctly. That is its own bug, tracked separately, so the name is narrowed to BMP multi-byte characters (umlauts 2 bytes, Japanese 3 bytes) which still exercise the byte-denominated length without tripping over it. Also updates the out-of-proc data collector's expected per-test-case attachment count, which follows the test count. MtpUnderVstestTests: 16/16 on both console axes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Consume the MTP client drop with the Content-Length framing fix Replaces the interim local-feed pack with a build of microsoft/testfx#10297, which stacks the Content-Length byte/char fix onto #10085. The transport now reads exactly Content-Length bytes and UTF-8-decodes them, symmetric with the write path, and reads the headers through the same byte-level buffer so no StreamReader can buffer part of the body across the boundary. That drop also carries #10085's ServerRequestHandler signature change (the result is now constrained to a serializable dictionary), so FakeMtpServerClient is updated to match. Verification on this drop: - CrossPlatEngine builds clean on net462, netstandard2.0 and net8.0. - MTP unit tests 140/140 (70 per axis, net11.0 and net481). - MtpUnderVstestTests 16/16 on both console axes. Note the 16/16: the two /logger:trx failures reported against the earlier drop do not reproduce here, so they look like a local deployment issue rather than anything in the retarget. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Repin the interim MTP client to the uniquely-named utf8fix1 drop Swaps the interim feed pack from the plain 2.4.0-dev build output to the coordinator's canonical 2.4.0-dev.utf8fix1 drop of microsoft/testfx#10297. Byte-equivalent content: all 184 contentFiles are identical between the two packs, including TcpMessageHandler.cs with both ReadExactlyAsync and the TrimPreamble BOM tolerance. Only the version metadata differs. The rename is the point. While the package is served from a committed local folder, NuGet caches by version, so a plain 2.4.0-dev risks silently resolving a stale cache entry from an earlier drop of the same name. The unique suffix makes that impossible, matching the convention the branch already used for 2.4.0-dev.numberfix. Re-verified from a cleared package cache: CrossPlatEngine clean on all three TFMs, MTP unit tests 140/140, MtpUnderVstestTests 16/16. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Address expert review feedback on the MTP hardening Localize the missing-uid error. The message reaches the user verbatim - StartTestRun funnels ex.Message into HandleLogMessage(Error) - and every other user-facing TestPlatformException in this assembly is resourced, so a hardcoded English string formatted with CurrentCulture was self-contradictory. Adds MtpTestCaseMissingNodeUid to Resources.resx, the generated designer property, and a trans-unit to all 13 xlf files. The text now also states the remedy (re-run discovery, or run without a selection) rather than only naming the failure, and the comment records that aborting the whole source is deliberate: silently running the addressable subset would recreate the same class of bug in a smaller form. Mark the three new test classes DoNotParallelize. MSTest parallelizes across classes at MethodLevel by default here, and these classes mutate process-global state - the MtpServerClientFactory.Launch seam and VSTEST_CONNECTION_TIMEOUT - so a save/restore in TestInitialize/TestCleanup could restore one class's value while another class's test was still relying on its own. That would have flaked in CI looking like a product bug. Close a hole in the float range guard. (float)int.MaxValue rounds *up* to 2147483648f, so comparing a float directly against int.MaxValue let that value through and the cast then saturated - precisely the plausible-looking wrong answer the guard exists to reject. Widen to double before comparing, and extend the regression test to cover it. Capture ProcessId before the exit handshake instead of reading it afterwards, when the process may already be gone. Test fixes: TryExitDoesNotUseAnAlreadyCancelledRunToken was vacuous (it built a cancelled token it never passed anywhere) and LaunchDefaultsToTheRealClientLauncher asserted only non-null, which any delegate satisfies. Both now assert something that fails if the behaviour regresses. Adds the missing mixed-selection case, where only some tests carry a uid. Also fixes a stale test-count comment and softens an overclaim in MtpPureProject, which no test currently references. Unit tests 142/142 across net11.0 and net481; MtpUnderVstestTests 16/16 on both console axes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Consume the latest MTP client drop from testfx#10297 Picks up the two commits that landed on the testfx branch after the utf8fix1 pack: the header line buffer is now reused across lines instead of allocated per line (server mode emits a notification per test, so that was a real hot-path allocation), plus comments recording why Content-Length is intentionally not capped and why the framing tests are not cross-TFM coverage. Both changes are to TcpMessageHandler, which compiles into CrossPlatEngine, so they are verified here rather than assumed. Re-verified from a cleared NuGet package cache: - CrossPlatEngine builds clean on net462, netstandard2.0 and net8.0. - MTP unit tests 142/142 across net11.0 and net481. - MtpUnderVstestTests 16/16 on both console axes. - testfx's own ServerClient unit tests 48/48, confirming the shared transport is still good on both formatter paths. The buffer is safe to hold as instance state for the same reason the existing read offsets are: reads are single-threaded, driven by exactly one read loop. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Consume the published MTP client package; drop the interim local feed testfx#10085 shipped the source-only MTP server-mode client to the dnceng-public dotnet-tools feed (already configured in NuGet.config), under its final name Microsoft.Testing.Platform.ServerMode.Client.Sources. Repin CrossPlatEngine from the interim local-feed drop (Microsoft.Testing.Platform.ServerClient.Source 2.4.0-dev.utf8fix2) to the published 2.4.0-preview.26410.1 and remove the whole interim scaffolding: - eng/local-mtp-feed and its NuGet.config source + .gitignore exception. - The GetNugetSourceParameters feed-order workaround in IntegrationTestBuild, which only existed to make a local-folder source restore alongside the remote https feeds. With no local folder it reverts to the simple base. The published package compiles its own down-level nullable-annotation polyfills on net462/netstandard2.0, which collide with the identical set CrossPlatEngine already imports from CoreUtilities (CS0436). Define MTP_CLIENT_EXCLUDE_NULLABLE_ATTRIBUTES so the package defers to those; it is a no-op on net8.0 where the attributes are in-box. The C# namespace (Microsoft.Testing.Platform.ServerMode.Client) is unchanged, so the retarget glue and azat's unit tests bind to the published package with no code change. Restore resolves 2.4.0-preview.26410.1 from the real feed with no local folder; build is clean on all three TFMs. 🤖 * Enable the MTP testhost in the non-ASCII acceptance test RunMtpApplicationPreservesNonAsciiTestNames drove the MTP app with a plain InvokeVsTest, which stopped detecting the app after main merged #16337 (MTP testhost disabled by default). Align it with every other MTP-driving test by using InvokeVsTestWithMtpTestHostEnabled, so the net11.0 runner finds the testhost again. net11.0 is back to a full pass; the remaining net481 /logger:trx failures are the pre-existing environmental logger-load issue on the desktop runner, unrelated to this change. 🤖 * Reject fractional MTP line numbers Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Reject selected MTP nodes without UIDs Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Azat Muzafarov <azatm@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f
2 participants
The bug
TcpMessageHandlerdeclaresContent-Lengthin UTF-8 bytes on the write path, but the read path consumed that number as decoded characters.Write path (correct, unchanged):
Read path before:
_readeris aStreamReader, soReadBlockAsyncyields decoded characters. The#else(Jsonite, net462/netstandard2.0) branch had the identical defect withchar[] commandChars = new char[commandSize].Read path after — consume exactly
Content-Lengthbytes, then UTF-8-decode, symmetric with the write path:For pure ASCII, bytes == characters, so nothing broke and the defect stayed latent. For any multi-byte UTF-8 content — a localized test name, an exception message, or captured standard output in German, Japanese, Czech, Cyrillic, or an emoji — the two counts disagree. The reader under-reads the frame, leaves the body's tail in the stream, and the framing desynchronizes permanently: the leftover bytes are parsed as the next frame's headers.
The symptom only appears from the second frame onward. The first frame still deserializes fine (its JSON is merely truncated at a point the parser may or may not reject); it is the following read that picks up mid-message garbage. Any regression test therefore has to send at least two messages.
Both compilation branches were affected —
#if NETCOREAPP(System.Text.Json, net8+) and#else(Jsonite, net462/netstandard2.0) — and both are fixed.This affects the live server, not just the new client package
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/TcpMessageHandler.csis the shipping MTP server-mode transport. The source-only client package added in #10085 compiles the same file as source, which is how the bug surfaced, but the defect is in the server transport itself and has always been there. Anything speaking server mode against a test host that emits non-ASCII test names or output could hit it.The
StreamReaderbuffering hazardReading the body as raw bytes from
_reader.BaseStreamwhile the headers still went throughStreamReaderwould have been a worse bug:StreamReaderfills its own internal buffer, so by the time the header line is returned it has typically already swallowed part of the body, andBaseStreamwould resume past it.The fix therefore removes
StreamReaderfrom the read path entirely. Headers and body are both read through a single byte-level buffer owned by the handler (_readBuffer/ReadHeaderLineAsync/ReadExactlyAsync/FillReadBufferAsync), so header and body reads sit on the same side of the buffering boundary and nothing can be stranded. This mirrors what vstest's hand-written client (the now-deletedMtpServerConnection, replaced in microsoft/vstest#16300) already did correctly: read the header line at the byte level, then read exactlycountbytes andEncoding.UTF8.GetStringthem.One compatibility detail worth calling out: the old
StreamReaderhad byte-order-mark detection enabled by default and silently swallowed a leading UTF-8 preamble. Some peers write their headers through a preamble-emitting encoder (new StreamWriter(stream, Encoding.UTF8)— the platform's ownServerTestsdo exactly this), so the new reader keeps that tolerance explicitly viaTrimPreamble, and there is a test for it. Cancellation semantics and theSocketError.ConnectionResetgraceful-disconnect handling are unchanged.Tests
The regression tests live in
test/UnitTests/Microsoft.Testing.Platform.UnitTests/ServerMode/TcpMessageHandlerTests.cs, merged into the existing test class. They stand up a pair ofTcpMessageHandlers joined over loopback TCP, so the bytes on the wire are exactly what a real server and client exchange. That project multi-targets net462 alongside net8.0/net9.0, so every test runs against both the#if NETCOREAPP(System.Text.Json) and#else(Jsonite) compilation branches.Covering the byte/char defect itself:
ReadAsync_RawUtf8FramesFromPeer_DoesNotDesynchronizeSubsequentFrames— two frames from a conformant peer that emits raw UTF-8, the first with multi-byte content. Fails before the fix on every TFM.ReadAsync_BodyLargerThanReadBuffer_SpansMultipleRefillsWithoutDesynchronizing— a body far larger than the read buffer, so it spans several refills; a second frame follows to prove the reader stopped on the exact byte boundary. Fails before the fix.ReadAsync_HandlerWrittenFramesWithNonAscii_DoesNotDesynchronizeSubsequentFrames— the same round trip throughTcpMessageHandleron both ends, using realclient/logandtelemetry/updatenotifications. See the caveat below: this one only exercises the defect on net462.ReadAsync_AsciiFrames_RoundTripInOrder— guards the ASCII fast path against over-reading.Covering the paths this change introduces:
ReadAsync_HeaderLineLongerThanInitialBuffer_GrowsAndParsesFrame— a 5 KB header line, driving the_headerLineBuffergrowth path through several doublings.ReadAsync_PeerDisconnectsMidBody_ReturnsNull— a peer that announces a body then hangs up mid-way, which must surface as a graceful disconnect rather than a hang or a throw. Fails before the fix.ReadAsync_LeadingByteOrderMark_IsSkipped— pins the BOM tolerance the oldStreamReaderprovided implicitly.Coverage caveat worth knowing
ReadAsync_HandlerWrittenFramesWithNonAscii_...cannot detect this bug on .NET.Utf8JsonWriterusesJavaScriptEncoder.Default, which escapes every non-ASCII character to\uXXXX, so on net8.0/net9.0 the body reaches the wire as pure ASCII, byte count equals char count, and the pre-fix reader passes it unchanged. It only exercises the regression on net462, where Jsonite emits BMP characters unescaped. The docstring says so outright.The cross-TFM coverage comes from the two tests that frame raw UTF-8 bytes themselves and so bypass formatter escaping entirely.
This is also why the bug stayed latent: MTP-to-MTP traffic is accidentally ASCII on the wire, so the defect only bites when the peer emits raw UTF-8 — which is exactly what an external client such as vstest's does.
Failure output before the fix
net8.0:
The
'C'is theCof the next frame'sContent-Lengthheader being read as part of the previous body — the desynchronization made visible.net462, where the non-ASCII tests hang until the read times out:
Verified by swapping
TcpMessageHandler.csback to its parent revision, rebuilding, and re-running — so these are genuine failing-before/passing-after regression tests, not tests fitted to the new implementation.Suite results (before → after)
Run locally in Release on Windows.
Microsoft.Testing.Platform.UnitTests(net8.0)Microsoft.Testing.Platform.UnitTests(net9.0)Microsoft.Testing.Platform.UnitTests(net462)ServerTests(live server over TCP)Microsoft.Testing.Platform.ServerClient.UnitTests(net8.0)Microsoft.Testing.Platform.ServerClient.UnitTests(net462)MtpServerClientSourcePackage*(contract + hostile consumer)MtpServerClientAcceptanceTests(real generated MTP app)Server*The platform suites are up 7 (the 4 transport tests plus 3 new buffer-path tests), and
ServerClient.UnitTestsis unchanged at 24.The
ServerTestsrow is the interesting one: it is the platform's own server-mode suite driving a realTestApplicationover TCP, and it is what caught the BOM regression in an intermediate version of this change.The fix is additionally validated outside this repo: a locally-built pack of the source package was consumed by the vstest retarget work, where CrossPlatEngine builds clean on net462/netstandard2.0/net8.0 and the MTP suites pass 140/140 and 16/16 — including the raw-UTF-8 peer traffic that exposes this bug.
Relationship to #10085
The
src/change is independent of #10085 and applies tomainas-is. The regression tests live inMicrosoft.Testing.Platform.UnitTests, which exists onmainand already multi-targets net462, so nothing about this fix requires the source-package work.Verified rather than assumed — rebasing all three commits onto current
main:TcpMessageHandlerTests.csis byte-identical onmainand on this PR's base;mainin the same files;TcpMessageHandler.csbetweenmainand this base is an unrelated 5-lineSubstring-vs-range-operator change from Add a source-only MTP server-mode client package #10085, which none of these commits touch.The base is left as
nohwnd-mtp-client-source-packageso it reads as an increment on that work, but it can be retargeted tomainat any time if the shipping-server fix should land independently.Provenance
This was raised by an automated reviewer on #10085 and acknowledged there by the author as a real latent bug, to be fixed in a dedicated platform PR rather than inside the packaging change. This is that PR, stacked on
nohwnd-mtp-client-source-packageso it reads as an increment on that work.