Harden the server-mode transport read path

[Evangelink]

Follow-up to #10297, from findings raised reviewing it. Based on nohwnd-mtp-client-source-package (where #10297 landed), not main.

Three of these are behavioral; the rest are comments and test hygiene.

1. A malformed Content-Length tore down the read loop

The comment #10297 added says commandSize is trusted unbounded, and that reasoning is right for large values. But negative and unparseable values aren't just unbounded — they escape.

Probed against the merged code:

Content-Length: -5              -> System.ArgumentOutOfRangeException:
                                   minimumLength ('-5') must be a non-negative value.
Content-Length: 99999999999999  -> System.Text.Json.JsonReaderException:
                                   The input does not contain any JSON tokens.
Content-Length: abc             -> (same JsonReaderException)
Content-Length:                 -> (same JsonReaderException)

Two distinct defects:

Negative — int.TryParse happily yields -5, the is -1 guard doesn't catch it, and ArrayPool.Rent(-5) (new byte[-5] → OverflowException on net462) throws straight past the SocketException/IOException filter. A malformed header kills the connection with an argument-validation exception rather than the graceful null every other malformed-frame path produces.

Unparseable — quieter and arguably worse. _ = int.TryParse(...) discards the result, and out contentSize is set to 0 on failure. So a header that failed to parse was read as a valid frame with an empty body, and only failed later as a JSON error pointing at the payload instead of at the header that was actually wrong.

Both now return -1, so ReadAsync returns null like every other unusable frame. Content-Length: 0 is untouched — it's well-formed framing and still reaches the formatter.

2. The read path transcoded bytes → chars → bytes

JsonDocument.Parse(ReadOnlyMemory<char>) immediately transcodes back to UTF-8 into its own pooled array:

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 every frame did GetCharCount + Rent<char> + GetChars, and STJ undid all three — while the raw UTF-8 bytes were already sitting in bodyBuffer. The formatter now takes ReadOnlyMemory<byte> on .NET and the body goes straight through. net462/Jsonite is unchanged (it only accepts a string).

This also retires the note that was sitting on IMessageFormatter asking for exactly this:

// Note: The current design might impose performance overhead, since the data
//       is not directly deserialized from a stream, but rather first a string is extracted
//       and allocated and then a string is deserialized.
//       We could create a version that uses System.Buffers APIs ...

and removes Deserialize<T>(ReadOnlyMemory<char> utf8Json) — a char buffer with a utf8 name, which is the byte/char ambiguity #10297 exists to fix, one layer down.

Lifetime note, since this is the subtle part. JsonDocument.Parse(ReadOnlyMemory<byte>) does not copy; it reads out of the buffer until the document is disposed. The pooled buffer therefore has to outlive the document, and it does — Deserialize disposes the document after Bind has fully materialized the graph, and only then does the finally return the buffer.

This requirement is not new: the char overload rents its own byte array and returns it on Dispose, so anything retaining a JsonElement past Deserialize would already have been broken. I checked all the deserializers — including the recursive IDictionary<string, object?> and object[] ones, which are the only plausible offenders — and every one materializes into owned objects (string, Dictionary, object[], primitives). Nothing boxes a JsonElement. Documented on both sides so it stays that way.

3. Comments that overstated their guarantee

The header-buffer comment justified unbounded growth by pointing at Content-Length's trust boundary. The two aren't equivalent: a body is legitimately unbounded (stack traces, captured stdout), which is what makes refusing to cap it correct; a header line has no legitimate large case, and a peer that never sends \n doubles the buffer forever. Still uncapped, but the comment now says which argument actually applies and that headers are the cheaper place for a cap if one is ever wanted.

Also documented that ReadHeaderLineAsync breaks only on LF, whereas the StreamReader it replaced also treated a lone CR as a terminator. Protocol-irrelevant and arguably stricter-is-better, but the comment read as if it enumerated the cases.

Tests

• ReadAsync_MalformedContentLength_ReturnsNull — negative, int.MinValue, larger-than-Int32, non-numeric, empty.
• ConnectedHandlers.CreateAsync no longer leaks the listener and a half-open socket if connect/accept throws. On a loopback-heavy suite that surfaces as port exhaustion in an unrelated test rather than as the failure that caused it.
• ReadAsync_LeadingByteOrderMark_IsSkipped is now ASCII-only. It carried NonAsciiMethod, so pre-Fix Content-Length byte/char mismatch in the server-mode TCP transport #10297 it failed for the byte/char reason rather than isolating BOM handling; it can now only fail if preamble handling itself breaks.
• FormatterUtilitiesTests gets a Deserialize<T>(string) helper hiding the per-TFM split, which removes eight #if NETCOREAPP blocks and four SA1009/SA1111 pragma pairs that only existed to work around a #if in the middle of a call expression.

Verification

Guard confirmed load-bearing by neutralizing just the if (!parsed || contentSize < 0) condition and re-running — all five rows fail, two with ArgumentOutOfRangeException and three with JsonReaderException, matching the probe above. Restoring it returns them to green.

Suite	Result
Microsoft.Testing.Platform.UnitTests (net8.0)	1452/1452
Microsoft.Testing.Platform.UnitTests (net9.0)	1452/1452
Microsoft.Testing.Platform.UnitTests (net462)	1428/1428
Microsoft.Testing.Platform.ServerClient.UnitTests (net8.0)	24/24
Microsoft.Testing.Platform.ServerClient.UnitTests (net462)	24/24
Full repo build (Debug)	0 warnings, 0 errors

Platform suites are up 5 from the 1447/1423 baseline #10297 established — the five new data rows. ServerClient.UnitTests is unchanged at 24, which matters because that project compiles TcpMessageHandler.cs, IMessageFormatter.cs and FormatterUtilities.cs as source and exercises the net462/Jsonite side of every #if touched here.

[github-actions]

🧪 Test quality grade — PR #10346

34 new test methods across 4 new files. 24 A-grade, 6 B-grade, 4 D-grade, no failures. The dominant issue is four unit tests that use Task.Delay(100) as a synchronization barrier — a wall-clock wait that makes the tests timing-sensitive and a potential source of flakiness on slow machines. The single highest-leverage improvement is replacing those delays with a proper signal-based mechanism (e.g., a TaskCompletionSource that fires when the pending request has been registered).

Grade	Test	Notes
D (60–69)	new MtpServerClientTests. ReadLoop_ MalformedFrame_ FailsPendingRequestWithClientException	Uses Task.Delay(100) to let the run request register before injecting the bad frame — replace with a signal-based wait to avoid timing flakiness.
D (60–69)	new MtpServerClientTests. ReadLoop_ ServerDisconnect_ FailsPendingRequestWithClosedException	Uses Task.Delay(100) to synchronize before closing the connection — replace with a signal-based wait.
D (60–69)	new MtpServerClientTests. RunTestsAsync_ Cancellation_ SendsCancelRequestAndThrows	Uses Task.Delay(100) to let the request register before cancelling — a TaskCompletionSource triggered when the request is pending would be flake-free.
D (60–69)	new MtpServerClientTests. TestNodesUpdated_ CompletionSentinel_ IsSkipped	Uses Task.Delay(100) for ordering between the sentinel and the real update — the existing WaitForNotificationAsync pattern or a TaskCompletionSource would be deterministic.
B (80–89)	new MtpServerClientTests. DiscoverTestsAsync_ All_ SendsDiscoverRequest	Single Assert.Contains on the method name; consider also asserting that no unexpected parameters were sent (or share the params-assertion pattern from the WithUids sibling).
B (80–89)	new MtpServerClientTests. ExitAsync_ SendsExitNotification	Single behavioral assertion via WaitForNotificationAsync; verifies the notification was sent but not its content — sufficient for this operation, no issues found beyond single-assertion breadth.
B (80–89)	new MtpServerClientTests. ServerInitiatedRequest_ NoHandler_ RespondsWithNull	Single Assert.IsNull(response.Result); null is the correct semantic outcome here, but asserting the response ID round-trips or no error field is set would add depth.
B (80–89)	new MtpServerClientTests. ServerInitiatedRequest_ WithHandler_ InvokesHandler	Single equality on observedMethod; consider also asserting the response was dispatched (e.g., the server received a null result response) to verify the full dispatch path.
B (80–89)	new MtpServerClientTests. TestNodesUpdated_ PassedNode_ DecodesExecutionState	Only one assertion (AreEqual on ExecutionState); the DiscoveredNode sibling asserts five fields — consider parity to catch partial-decode regressions.
B (80–89)	new MtpServerClientSourcePackageConsumerTests. HostileConsumer_ CompilesAgainstPackedSource	Single exit-code equality assertion — correct and sufficient for a compile oracle; no improvements needed.
A (90–100)	new MtpServerClientAcceptanceTests. DiscoverAndRun_ ViaSourcePackageClient_ ReportsExpectedTestNode	Two independent sessions, ContainsSingle on exact node properties for both discover and run paths; thorough end-to-end coverage.
A (90–100)	new MtpServerClientTests. AttachmentsReceived_ DecodesAttachments	No issues found.
A (90–100)	new MtpServerClientTests. DiscoverTestsAsync_ WithUids_ SendsDiscoverRequest	No issues found.
A (90–100)	new MtpServerClientTests. DiscoverTestsWithFilterAsync_ SendsDiscoverRequest	No issues found.
A (90–100)	new MtpServerClientTests. InitializeAsync_ DecodesServerCapabilities	Exhaustive field-by-field equality over all capability properties including the cached AreSame check. No issues found.
A (90–100)	new MtpServerClientTests. LogReceived_ DecodesLevelAndMessage	No issues found.
A (90–100)	new MtpServerClientTests. RunTestsAsync_ All_ SendsRunRequest	No issues found.
A (90–100)	new MtpServerClientTests. RunTestsAsync_ MapsArtifactsToAttachments	No issues found.
A (90–100)	new MtpServerClientTests. RunTestsAsync_ WithUids_ SendsRunRequest	No issues found.
A (90–100)	new MtpServerClientTests. RunTestsWithFilterAsync_ SendsRunRequest	No issues found.
A (90–100)	new MtpServerClientTests. ServerInitiatedRequest_ WithNonDictionaryIDictionaryResult_ RoundTripsResultOverTheWire	No issues found.
A (90–100)	new MtpServerClientTests. ServerInitiatedRequest_ WithNonNullDictionaryResult_ RoundTripsResultOverTheWire	No issues found.
A (90–100)	new MtpServerClientTests. TelemetryReceived_ DecodesEventNameAndMetrics	No issues found.
A (90–100)	new MtpServerClientTests. TestNodesUpdated_ DiscoveredNode_ DecodesNodeAndRunId	No issues found.
A (90–100)	new MtpServerClientTests. TestNodesUpdated_ FailedNode_ DecodesErrorMessage	No issues found.
A (90–100)	new MtpServerClientTests. TestNodesUpdated_ PassedNodeWithDetails_ DecodesOutputAndLocation	No issues found.
A (90–100)	new MtpServerClientSourcePackageTests. SourcePackage_ ContainsNoCompiledOutput	No issues found.
A (90–100)	new MtpServerClientSourcePackageTests. SourcePackage_ EveryCsFileIsCompileContentFile_ AndManifestMatchesPackedFiles	No issues found.
A (90–100)	new MtpServerClientSourcePackageTests. SourcePackage_ JsoniteNamespace_ IsPackageQualified_ NotTopLevel	No issues found.
A (90–100)	new MtpServerClientSourcePackageTests. SourcePackage_ NetStandardJsonPath_ IsJsoniteOnly_ AndNetIsSuperset	No issues found.
A (90–100)	new MtpServerClientSourcePackageTests. SourcePackage_ ShippedLinkedAndClientSource_ IsAutoGeneratedAndInternal	No issues found.
A (90–100)	new MtpServerClientSourcePackageTests. SourcePackage_ ShipsClientApi_ InEveryTargetFramework	No issues found.
A (90–100)	new MtpServerClientSourcePackageTests. SourcePackage_ ShipsBuildTargets_ AndNet462SafetyGuardsSurviveTransform	No issues found.
A (90–100)	new MtpServerClientSourcePackageTests. SourcePackage_ ShipsPolyfills_ AndDoesNotLeakBuildGeneratedSource	No issues found.

_{This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Re-run with
/grade-tests.}

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

🤖 Automated content by GitHub Copilot. Generated by the Grade Tests on PR (on open / sync) workflow. · 101.7 AIC · ⌖ 5.37 AIC · ⊞ 8.9K · [◷]( · ◷)