Harden the MTP client: UTF-8 framing, shutdown, uid filter, and tests

[azat-msft]

Stacked on your nohwnd-mtp-source-client-integration branch — targeting it directly rather than main so it reads as an increment on your work and you can take it as a fast-forward.

Started as a review of microsoft#16300, then turned into fixes for what the review turned up. Companion testfx PR: microsoft/testfx#10297 (stacked on microsoft#10085).

1. A regression the retarget introduces: UTF-8 frame desynchronization

The single most important item, because it is a step backwards rather than an inherited flaw.

MTP frames declare Content-Length in UTF-8 bytes. The package's TcpMessageHandler writes it that way:

int byteCount = Encoding.UTF8.GetByteCount(messageStr);
await _writer.WriteLineAsync($"Content-Length: {byteCount}");

but reads it as a character count:

char[] commandCharsBuffer = ArrayPool<char>.Shared.Rent(commandSize);
await _reader.ReadBlockAsync(memoryBuffer, cancellationToken);

For any frame carrying multi-byte UTF-8 the two disagree: the reader under-reads, leaves the body's tail in the stream, and the framing desynchronizes from the next message onward.

The deleted MtpServerConnection was byte-correct here — it read count bytes into a byte[] and then Encoding.UTF8.GetString(body) — so this is newly introduced, not pre-existing.

Fixed upstream in microsoft/testfx#10297: read exactly Content-Length bytes and decode, and read headers through the same byte-level buffer so no StreamReader can swallow part of the body across the boundary. This PR consumes that drop.

Two findings worth recording, because they change how the bug should be described:

• It is not reachable end to end through a .NET MTP app today. The server serializes with System.Text.Json, whose default encoder escapes non-ASCII to \uXXXX, so the bytes on the wire are ASCII and byte count coincidentally equals character count. It has to be proved at the unit level against the transport — which is what the testfx PR does. The acceptance test here is a name-integrity guard and its comments say so rather than overclaiming.
• A separate defect surfaced while writing it. Astral-plane characters are escaped as a surrogate pair and arrive in the TRX as the literal text \ud83c\udf89 instead of the character; BMP characters decode correctly. I have not chased it down and it is not addressed here — flagging it as its own issue. The test name is narrowed to BMP multi-byte characters so it does not trip over this.

2. Shutdown: cancellation and an unbounded wait

Exit changed from a fire-and-forget notification into an awaited request/response, which introduced two problems:

• It was awaited on the run's own cancellation token. Cancelling or aborting is exactly when that token is already cancelled, so ExitAsync threw immediately and the graceful handshake was skipped in the one case it matters most.
• The await was unbounded. A test application that never acknowledges exit would hang discovery or execution indefinitely; the notification it replaced could not block at all.

Both managers now go through MtpServerClientFactory.TryExit, which runs on its own bounded token, swallows failures (the caller disposes next, which tears the process down anyway), and is called from a finally so a failed or cancelled run still shuts down.

The factory also exposes a replaceable Launch delegate purely so the managers can be driven against a fake server in tests. Production always uses MtpServerClient.Launch. No public API added.

3. A missing node uid silently ran zero tests

BuildUids substituted FullyQualifiedName when a TestCase carried no MTP.TestNode.Uid. Per G3 the server projects node.Uid alone and never reads any other field, so that substitution produces a filter matching nothing: the run reports success having executed zero of the tests the user selected, with no error anywhere.

Now throws, with a comment at the site explaining why no fallback is correct — the fallback looks defensive, so without the rationale written down someone will restore it.

(The good news: I checked, and the uid does survive cross-process serialization. JsoniteConvert round-trips custom properties and auto-registers unknown ones by id, so the VS / AzDO "run selected tests" path is fine. The defect was only the fallback.)

4. Coercion fixes

• TryGetRawInt used unchecked((int)l), wrapping out-of-range values into a plausible-looking wrong line number. Now range-checked, so the property stays at its visibly-unset default.
• AddTraits collapsed every non-string trait value to string.Empty. Since the formatters box JSON scalars differently, a numeric or boolean trait was silently dropped on one formatter and kept on the other. Now formatted invariantly.

5. Deduplicated the connection timeout

MtpClientOptionsFactory re-read VSTEST_CONNECTION_TIMEOUT and hardcoded the 90-second default. EnvironmentHelper.GetConnectionTimeout() already owns this, is used by seven other call sites, and also traces the override. Carried over from the deleted MtpClientHelpers, but this is a new file, so it seemed the moment to stop copying the literals forward.

6. Tests

There were none for any MTP code. The retarget deletes the transport and leaves behind pure, dependency-free conversion logic, which is ideal to test directly.

85 new unit tests, covering the normalized-Node contract and per-formatter number boxing (G1), outcome mapping, the action-node filter, bridge properties, standard output/error, traits, duration, log levels, the env-var casing behaviour (G5), the uid filter (G3), and both manager flows against a fake server.

Worth noting these turn three of the five G-items from "confirmed verbally by the package owner" into executable assertions that keep holding across future package drops.

Verification

• 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.

On that last number: the two /logger:trx failures you saw do not reproduce here, which supports your read that they are a local deployment issue rather than anything in the retarget.

Still interim

eng/local-mtp-feed now holds a local build of testfx#10297, pinned as 2.4.0-dev.utf8fix1 — uniquely named so NuGet cannot silently resolve a stale cache entry while the package is served from a committed folder, matching the convention the branch already used for 2.4.0-dev.numberfix. All of it still comes out once microsoft#10085 ships to a real feed.

One suggestion unrelated to the above: the IntegrationTestBuild.GetNugetSourceParameters change is labelled interim, but the rewrite from .Descendants("add") + attribute scraping to .Elements("add") + .Attribute("value") fixes a latent bug independent of the local feed, and the reordering becomes a no-op once the feed is gone. Might be worth keeping permanently and splitting into its own commit that can merge ahead of everything else.