Add in-process launch support to the MTP server-mode client by Evangelink · Pull Request #10898 · microsoft/testfx

Amaury Levé (Evangelink) · 2026-08-31T19:22:25Z

Why

MtpServerClient.LaunchAsync(path) owns the loopback listener and starts the test application with Process.Start. That is right for IDE and desktop tooling, but unusable for embedded hosts — MAUI apps, Android/iOS test apps — where the MTP application already runs in the caller's process.

Today those hosts have to reimplement the listener, the --server/host/port arguments, the race between connection and startup failure, the serializer-before-formatter registration order, TcpMessageHandler + MtpJsonRpcConnection construction, and the exit / transport-close / server-completion shutdown dance. That defeats the point of shipping a canonical client and makes it easy to reintroduce bugs this package already solved: partial frame writes, ignored JSON-RPC errors, missing $/cancelRequest, unbounded waits, exception masking.

A concrete consumer is the DeviceRunners MSTest visual-runner work (mattleibow/DeviceRunners#157, #9809).

What

usingIMtpServerClientclient=awaitMtpServerClient.LaunchInProcessAsync(async(serverArgs,token)=>{ITestApplicationBuilderbuilder=awaitTestApplication.CreateBuilderAsync(serverArgs);builder.AddMSTest(()=>testAssemblies);usingITestApplicationapp=awaitbuilder.BuildAsync();returnawaitapp.RunAsync();},options,cancellationToken);awaitclient.InitializeAsync();awaitclient.DiscoverTestsAsync();awaitclient.RunTestsAsync();awaitclient.ExitAsync();awaitclient.ShutdownAsync();

The caller supplies only "how to run the application". The client keeps everything else: it binds the listener, generates the complete argument array (--server jsonrpc --client-host 127.0.0.1 --client-port <port> --no-banner), races the connect, builds the transport, and owns a bounded shutdown.

New API on the injected (still internal) surface:

Member	Purpose
`MtpServerClient.LaunchInProcessAsync(callback, options, ct)`	The embedded-host launch path. Async only — blocking the launching thread can deadlock the application being launched.
`IMtpServerClient.ShutdownAsync()`	Non-blocking teardown.
`IMtpServerClient.ServerExitCode`	The value the application returned.
`MtpServerClientOptions.ServerShutdownTimeout`	Graceful shutdown bound (default 30s).

Internally, the transport setup was extracted from MtpServerProcess into MtpServerConnector, and both launch paths now sit behind IMtpServerHost, so they cannot drift.

Behavior worth reviewing

Ownership and shutdown. Both Dispose() and ShutdownAsync() join one lazily created shared teardown task, on both hosts. A Dispose that follows or races ShutdownAsync therefore returns only once the server has actually stopped, rather than reporting success while teardown is still running. Teardown runs on the thread pool, so ShutdownAsync never blocks and the synchronous Dispose cannot deadlock against a UI-thread continuation. Task.Run captures the execution context, so the connection's read-loop AsyncLocal marker still flows and disposing from a notification handler does not self-wait (covered by a test asserting < 4s against the connection's 5s read-loop timeout).

Bounded, and actually bounded. Teardown waits ServerShutdownTimeout, then cancels the callback's token, then a fixed 5s grace, then abandons and logs. CancellationTokenSource.Cancel() runs registrations synchronously, so the cancellation is started separately — otherwise a blocking caller registration would prevent the grace from ever starting and make the "bounded" wait unbounded. A failed launch skips the graceful wait entirely: nothing is connected, so there is no transport closure for the callback to observe.

Exception preservation. A callback that throws, is canceled, or returns before dialing back surfaces as MtpServerConnectionClosedException with the original as InnerException, instead of a misleading connection timeout. Every teardown helper is non-throwing, and the shared teardown task is wrapped so it can never fault — a faulted shared task would throw from every later disposal.

LaunchAsync(path) is unchanged. Verified against main: identical argument string, failure messages, stderr capture, exit fast-fail and teardown order. One earlier revision of this branch added a grace period to the accept race; review showed it only traded a precise failure for a vague one, so it was removed and the external path is now provably unchanged.

Browser/WASM. Both paths are loopback TCP. LaunchInProcessAsync fails fast with PlatformNotSupportedException there; this does not enable WASM hosting, and the docs say so.

Tests

68 unit tests (net8.0) / 63 (net462, Jsonite path). New coverage: argument array shape; initialize → discover → run → exit; callback faulting synchronously and asynchronously; callback exiting with a code; null task; pre-canceled launch not invoking the callback; cancellation during connect canceling the callback token; connection timeout bounded by the grace rather than ServerShutdownTimeout; ShutdownAsync; ServerExitCode; disposal awaiting the callback; Dispose racing an in-flight ShutdownAsync; Dispose from a notification handler; idempotent disposal; callback faulting during shutdown; unresponsive callback abandoned within the bound; $/cancelRequest; stateful on/off; multi-request single connection; EnvironmentVariables ignored and warned. Stressed 6×/5× consecutively for flakiness.
New acceptance test where a single generated process is simultaneously the embedded host (compiling the packed source-only package) and a real MTP TestApplication with real MSTest over a real [TestClass] — discovering and running its own test over JSON-RPC with no Process.Start anywhere. It also asserts the server's reported process id equals its own.
Existing external-process acceptance test extended to cover ShutdownAsync + ServerExitCode; hostile-consumer compile oracle extended to bind the new API on net462/netstandard2.0/net5.0–net8.0.
Full regression: 13 platform source-package/consumer acceptance, 4 MSTest acceptance, 13 platform ServerTests. build.cmd -pack clean, 0 warnings.

Review history

Four independent review rounds (MTP/MSTest expert reviewer, a design reviewer, and two correctness reviewers) produced 3 major, 10 moderate and 10 minor findings, all addressed. Several were real bugs the tests then locked in — notably Process.ExitCode being unreadable after Process.Dispose(), a TcpListener socket leak when Start() failed, and a teardown path that could throw from a contract documented never to throw.

Open design questions

No IAsyncDisposable. netstandard2.0/net462 would need Microsoft.Bcl.AsyncInterfaces, breaking the package's dependency-free promise. ShutdownAsync() is the substitute. Happy to revisit if the dependency is acceptable.
MtpServerClientOptions is mode-mixed.EnvironmentVariables is external-process only; ServerShutdownTimeout is in-process only. Nesting per-transport options would age better, but EnvironmentVariables already shipped at the top level, so it cannot be done non-breakingly now.
The 5s cancellation grace is a fixed constant, not an option.
The two paths still build different argument shapes — deliberate, to keep the shipped external command line byte-identical. Commented at the call site.
No ConnectAsync(TcpClient/Stream, options) factory — the existing MtpServerClient(MtpJsonRpcConnection, options) constructor already covers "wrap an existing transport", and the guidance is to minimize injected surface. Easy to add if wanted.

Embedded hosts such as MAUI or Android/iOS test apps cannot spawn a child process, so `MtpServerClient.LaunchAsync(path)` was unusable for them and they had to reimplement the listener, the server-mode arguments, the connect race, the serializer/formatter ordering, the transport and the shutdown coordination by hand -- reintroducing the bugs the canonical client already solves. Add `MtpServerClient.LaunchInProcessAsync(callback, options, cancellationToken)`. The client keeps ownership of everything except "how to run the application": it binds the loopback listener, generates the complete server-mode argument array, races the connect against callback failure/completion, caller cancellation and the connection timeout, and owns a bounded shutdown that closes the transport, then cancels the callback token, then abandons it rather than hanging the caller. A callback failure before connection is surfaced as `MtpServerConnectionClosedException` with the caller's exception preserved as the inner exception, and teardown failures are only logged so they can never replace the primary failure. The shared transport setup is extracted from `MtpServerProcess` into `MtpServerConnector` and both launch paths now flow through `IMtpServerHost`, so the external-process behavior is unchanged while the two paths cannot drift. The one behavior change is an improvement shared by both: when the server is seen to have stopped, a still-pending accept gets a bounded final grace so a connection established just before the stop is not discarded in favor of a misleading "stopped before connecting back" failure. The path is loopback TCP, so it fails fast with `PlatformNotSupportedException` on browser/WASM; it does not enable WASM hosting. Fixes#10890 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Two independent reviews of the in-process launch path raised the same top finding: `Dispose()` waits for the hosted application synchronously on the calling thread, and on the very platforms this feature targets (MAUI, Android, iOS) a multi-second block on the UI thread trips the ANR / watchdog. Add `IMtpServerClient.ShutdownAsync()` (and `IMtpServerHost.ShutdownAsync`) so the same teardown can be awaited instead. `IAsyncDisposable` stays rejected -- netstandard2.0 and net462 would need `Microsoft.Bcl.AsyncInterfaces`, which breaks the package's dependency-free promise -- but rejecting the interface is not a reason to have no asynchronous shutdown at all. `Dispose()` remains, is still idempotent, and returns immediately after `ShutdownAsync`. Remove the accept grace period. The previous commit let a still-pending accept win for ~100ms after the server was seen to have stopped, on the theory that a connection established just before the stop should not be discarded. That theory does not hold: if the server has stopped, the socket belongs to a dead peer, so the grace only traded a precise failure (exit code plus captured stderr) for a generic connection-closed error on the first request, and its uncancellable delay could let the server failure beat a concurrent caller cancellation. Dropping it also makes the external-process path provably unchanged, so the changelog no longer needs a `Changed` entry. Other review fixes: * Expose `IMtpServerClient.ServerExitCode`. The callback signature already demanded a `Task<int>`, but after a successful session the value was unreachable, so an embedded host whose `Main` must return it had to capture it in a closure. * Do not dispose the server's `CancellationTokenSource` when the callback was abandoned while still running: it holds the token, and `token.WaitHandle` or `CreateLinkedTokenSource` would then throw `ObjectDisposedException` inside the caller's own code. * Skip the graceful wait entirely on a failed launch. Nothing is connected, so there is no transport closure for the callback to observe; only the fixed cancellation grace applies and an unwinding caller no longer pays `ServerShutdownTimeout`. * Clamp bounded waits instead of trusting the caller's `TimeSpan`. A negative value (or `Timeout.InfiniteTimeSpan`) made `Task.Delay` throw from a path documented never to throw; an oversized one is capped to the largest delay .NET Framework accepts. * Move `TcpListener.Start()` inside the cleanup `try`: it creates the socket before binding, so a bind failure leaked it because the caller never received a listener it could stop. * Log a late failure from an abandoned callback rather than only observing it. * Cache the in-process host's `ProcessId` instead of allocating a `Process` per property read. * Document that `Dispose()` blocks, that cancellation is bounded rather than immediate, and why the two launch paths still build different argument shapes; fix the shadowed `cancellationToken` in the PACKAGE.md sample. Tests: cover `ShutdownAsync`, `ServerExitCode`, and fail-fast on a server that stops without connecting; make `Dispose_IsIdempotent` able to fail by asserting on observed transport closes rather than callback invocations; replace the hand-rolled throws helper with `Assert.ThrowsExactlyAsync`; tighten the shutdown-bound assertion; and stop leaking `Process` handles. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

A third review round found three ways the teardown contract did not hold up under concurrency. `Dispose()` and `ShutdownAsync()` each had their own `Interlocked.Exchange` guard, so whichever call lost the race returned while teardown was still running -- reporting to its caller that the application had stopped when it had not. Replace both guards with one lazily started, shared teardown task: every entry point now joins the same task, so `Dispose` after (or racing) `ShutdownAsync` returns only once the application has actually stopped, and the whole thing stays idempotent. `MtpServerClient` no longer keeps its own guard either; it delegates to the host, whose teardown is the single source of truth. `ShutdownAsync()` could still block the very thread it exists to protect: it closed the transport before its first await, and closing the connection waits up to five seconds for the read loop. The shared task is started with `Task.Run`, so the whole teardown -- transport close included -- runs on the thread pool and `ShutdownAsync` returns immediately. `MtpServerProcess` likewise now runs its (bounded but synchronous) kill off the calling thread rather than pretending to be async while blocking. `CancellationTokenSource.Cancel()` executes registrations synchronously on the calling thread, so a caller registration that blocked would prevent the five-second cancellation grace from ever starting and make the "bounded" shutdown unbounded. Start the cancellation separately and begin the grace regardless. When a registration is still executing once the callback has finished, the source is reported as unsafe to dispose for the same reason an abandoned callback is: leaking one `CancellationTokenSource` beats a use-after-dispose inside the caller's code. Adds a test that a `Dispose` racing an in-flight `ShutdownAsync` blocks until the shared teardown completes, and that both share one teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

… fix ExitCode A fourth review round found that only the in-process host had been converted to the shared-teardown design. `MtpServerProcess` kept its old early-return guard, so on the external-process path a `Dispose()` that followed or raced `ShutdownAsync()` returned while `SafeKill`'s bounded `WaitForExit` was still running on another thread -- losing exactly the guarantee that wait exists for (a caller may delete the application directory immediately after disposal), and making `await ShutdownAsync()` report that the server had stopped when it had not. That contradicted the comment the previous commit added to `MtpServerClient.Dispose`, which claimed both paths joined an in-flight teardown. Convert `MtpServerProcess` to the same lazily created shared task, so the contract is now uniform across both implementations of `IMtpServerHost`, and reconcile the interface docs, which previously described the opposite rule. Adding real coverage for `ShutdownAsync` on the external-process path then surfaced a genuine bug: `MtpServerProcess.ExitCode` was always `null` after teardown, because a `Process` cannot be queried once disposed. Capture the exit code during teardown instead -- before the kill, so an application that already exited on its own reports its real code rather than the kill's, and before `Process.Dispose()`, after which nothing is readable. Also fixes a race in the in-process test fixture that made the suite flaky on net462: the callback published its `FakeMtpServer` only after its own connect call returned, but the client's accept can complete first, so a test could reach `Value` before the callback had set it. The fixture now exposes a `Connected` signal the launch helper awaits. 6/6 clean net462 runs and 5/5 clean net8.0 runs afterwards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

+ if (serverTask is not null)
+ {
+ // The launch is being abandoned, so skip the graceful wait entirely: there is no connected
+ // transport whose closure could signal the callback, and the caller (often a canceling one) is
+ // waiting on this unwind. A zero graceful timeout goes straight to cancel-then-grace.
+ if (!await ShutdownServerAsync(serverTask, serverCancellation!, TimeSpan.Zero, logger).ConfigureAwait(false))
+ {
+ // The callback is still running and still holds the token; disposing its source now would
+ // turn a clean abandonment into an ObjectDisposedException inside the caller's own code.
+ throw;
+ }
+ }


github-actions · 2026-08-31T19:32:23Z

🧪 Expert test review — PR #10898

Grade	Test	Mutation	Notes	How to improve
B (80–89)	new `MtpServerClientInProcessTests. Dispose_CallbackFaultsDuringShutdown_ DoesNotThrow`	2/3 killed	Reads `server.Completion.Exception` but never asserts on it, so a mutation changing the faulted exception's identity/message survives.	Assert the captured exception's message/type instead of discarding it after touching `.Exception`.
B (80–89)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_HonorsTheStatefulOption`	3/4 killed	Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on `client.Capabilities` is not caught.	Also assert `client.Capabilities.IsStateful` reflects the negotiated round-trip value, not only the sent request.
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_PassesCompleteServerModeArguments`	5/5 killed	Verifies exact ordered arguments, dynamic port and fixed count; strong regression guard for the generated argument array.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_DrivesInitializeDiscoverRunAndExit`	6/6 killed	End-to-end drive through initialize/discover/run/exit with distinct assertions per state transition.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsBeforeConnecting_ PreservesCallbackException`	2/2 killed	Asserts the exact exception instance is preserved as inner exception, not just its type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsAsynchronouslyBeforeConnecting_ PreservesCallbackException`	2/2 killed	Covers the async-throw variant with the same identity assertion as the sync case.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsWithoutConnecting_ FailsFastInsteadOfWaitingOutTheTimeout`	3/3 killed	Checks both the reported exit code in the message and a timing bound guarding against a slow-timeout regression.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsBeforeConnecting_ ReportsExitCode`	2/2 killed	Asserts the specific exit code surfaces in the exception message.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackReturnsNullTask_Fails`	2/2 killed	Asserts the specific inner exception type for a misbehaving callback returning a null task.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_NullCallback_Throws`	1/1 killed	Focused single-assertion guard-clause test using the exact exception type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_AlreadyCanceled_ DoesNotInvokeCallback`	2/2 killed	Verifies both the cancellation exception and, via an interlocked counter, that the callback never ran.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CanceledWhileConnecting_ CancelsTheCallbackToken`	2/2 killed	Confirms both the caller-side cancellation and that the callback's own token observed cancellation.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_ConnectionTimeoutElapses_ FailsWithTimeoutMessage`	3/3 killed	Distinguishes the connection timeout from the deliberately huge shutdown timeout via message and elapsed-time bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_ClosesTransportAndAwaitsTheCallback`	4/4 killed	Checks pre/post completion state, the callback's returned exit code, and the client's reported exit code.	—
A (90–100)	new `MtpServerClientInProcessTests. ShutdownAsync_ClosesTransportAndAwaitsTheCallback_ WithoutBlocking`	4/4 killed	Verifies await-completion, exit code, a fast follow-up Dispose, and single-teardown count together.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_FromANotificationHandler_ DoesNotSelfWaitOnTheReadLoop`	1/1 killed	Targeted timing assertion guards against a specific re-entrancy deadlock regression.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_WhileShutdownAsyncIsInFlight_ WaitsForTheSameTeardown`	3/3 killed	Uses a controlled release gate to prove the racing Dispose genuinely blocks, then confirms a single shared teardown.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_IsIdempotent`	3/3 killed	Confirms exactly one transport close and one completion despite three Dispose calls, plus a fast-return bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_CallbackIgnoresShutdown_ ReturnsWithinTheDocumentedBound`	2/2 killed	Verifies both the documented abandonment time bound and the logged "abandoning" diagnostic.	—
A (90–100)	new `MtpServerClientInProcessTests. RunTestsAsync_Canceled_ SendsCancelRequestToTheHostedApplication`	2/2 killed	Confirms both the client-side cancellation exception and the wire-level cancel notification reaching the fake server.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_MultipleRequestsOnOneSession_ ReuseTheSameConnection`	3/3 killed	Checks keep-alive negotiation, single-connection reuse count, and total request count together.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_IgnoresEnvironmentVariablesAndWarns`	1/1 killed	Asserts the specific warning content naming the ignored option.	—
A (90–100)	new `MtpServerClientInProcessAcceptanceTests. InProcessHost_ DiscoversAndRunsItsOwnMSTestNodes_ WithoutStartingAProcess`	5/5 killed	Real end-to-end embedded-host run asserting build success, exit code, and each marker line the generated app emits.	—
A (90–100)	mod `MtpServerClientAcceptanceTests. DiscoverAndRun_ViaSourcePackageClient_ ReportsExpectedTestNode`	1/1 killed	New lines assert the external-process ShutdownAsync/Dispose share a teardown and report an exit code.	—
A (90–100)	mod `MtpServerClientSourcePackageConsumerTests. HostileConsumer_ CompilesAgainstPackedSource`	N/A	Compile-only guard; added lines correctly extend surface coverage to `ShutdownAsync` and the in-process launch path.	—

_{This advisory comment was generated automatically. Grades are heuristic

and informational — they do not block merging. Suggestions on the Files

changed tab can be applied with one click. Re-run with
/review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · [◷]( · ◷)

github-actions · 2026-08-31T19:32:24Z

🧵 Parallel-safety audit — PR #10898

Parallelization — audited assemblies:

Test assembly	Scope	Workers	Analyzer coverage
`MSTest.Acceptance.IntegrationTests`	`MethodLevel` (default `[assembly: Parallelize(Scope = ExecutionScope.MethodLevel, Workers = 0)]` in `Program.cs`)	CPU count	coverable once the parallel-safety analyzers ship (attribute-based opt-in)
`Microsoft.Testing.Platform.Acceptance.IntegrationTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped
`Microsoft.Testing.Platform.ServerMode.Client.Sources.UnitTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped

This PR did not touch any .runsettings/testconfig.json/Directory.Build.* parallelization config — no assembly's scope changed.

Findings: A (global-state) 0 · B (paths) 0 · C (declaration) 0 · D (over-serialization) 0 — by severity: Critical 0 · High 0 · Warning 0 · Info 0.

No unsafe call sites found in the lines this PR added or modified.

MtpServerClientAcceptanceTests.cs — added lines only call client.ShutdownAsync() / assert ServerExitCode; no process-global state touched.
MtpServerClientSourcePackageConsumerTests.cs — added lines call into an isolated generated test asset (DriveAsync/DriveInProcessAsync); no shared/relative paths, env vars, culture, or console state mutated at the assembly level (Console.WriteLine here writes to the generated app's own redirected output, not the auditing process).
MtpServerClientInProcessAcceptanceTests.cs (new) — uses TestAsset.GenerateAssetAsync, which allocates a unique GUID-suffixed TempDirectory per instance (TestAsset.cs:19-26, throws if a path already exists) — no shared-path collision. CreateChildEnvironment() builds a fresh Dictionary passed only to the spawned child process's environment, not Environment.SetEnvironmentVariable, so it does not mutate this process's state.
FakeMtpServer.cs / MtpServerClientInProcessTests.cs (new) — each test constructs its own TcpListener/FakeMtpServer bound to an OS-assigned loopback port (new TcpListener(IPAddress.Loopback, 0)), so there is no fixed-port collision between concurrently-running tests; no static/process-global state is written.

Nothing here implies a testability or smell concern beyond this audit's scope.

_{Advisory only — heuristic, non-blocking. Re-run with /parallel-audit. This audit answers "is it parallel-safe?"; for testability, smells, or flakiness see the detect-static-dependencies / test-smell-detection / test-anti-patterns analyses.}

🤖 Automated content by GitHub Copilot. Generated by the Parallel-safety audit on PR (on open / sync) workflow. · auto · 117.1 AIC · ⌖ 3.53 AIC · ⊞ 24.8K · [◷]( · ◷)

github-actions

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · ◷

github-actions · 2026-08-31T19:32:25Z

+ client.Dispose();
+
+ Assert.IsTrue(server.Completion.IsFaulted);
+ _ = server.Completion.Exception;


🧪 Test review · Grade B (80–89) — Reads server.Completion.Exception but never asserts on it, so a mutation changing the faulted exception's identity/message would survive.
Assert the captured exception's message/type instead of discarding it after touching .Exception.
Suggested change
_=server.Completion.Exception;
Exception?faultException=server.Completion.Exception?.GetBaseException();
Assert.IsInstanceOfType<InvalidOperationException>(faultException);
Assert.AreEqual("The application failed while shutting down.",faultException!.Message);

github-actions · 2026-08-31T19:32:25Z

+ ? typed
+ : SerializerUtilities.Deserialize<InitializeRequestArgs>((IDictionary<string, object?>)initialize.Params!);
+
+ Assert.AreEqual(isStateful, args.Capabilities.IsStateful, "The in-process path must forward the client's stateful capability unchanged.");


🧪 Test review · Grade B (80–89) — Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on client.Capabilities is not caught.
Also assert client.Capabilities.IsStateful reflects the negotiated round-trip value, not only the sent request.
Suggested change
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,capabilities.IsStateful,"The negotiated capability the client stores must reflect the server's round trip, not just the sent request.");

github-actions

Note

🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

✅ 22/22 dimensions clean — no findings.

Summary: This PR adds MtpServerClient.LaunchInProcessAsync for embedded hosts (MAUI, Android/iOS) that cannot Process.Start. The implementation is thorough and well-structured:

Threading & Concurrency: Shutdown is idempotent via lock + lazy Task, Volatile.Read/Write for cross-thread exit code, Task.Run avoids sync-context deadlocks. No shared mutable state without synchronization.
Security & IPC: Loopback TCP only, no command-line injection surface (arguments are array-based), WASM fails fast with PlatformNotSupportedException.
Public API: All new API is internal (source-only package). No init accessors. No PublicAPI.Unshipped.txt changes needed.
Performance: GetCurrentProcessId() snapshotted once in constructor. No hot-path allocations. WaitBoundedAsync clamps timeouts correctly.
Cross-TFM: #if NET8_0_OR_GREATER guard on AcceptTcpClientAsync(CancellationToken). RuntimeInformation used for OS detection on net462.
Resource Management: Every disposable (TcpListener, TcpClient, CancellationTokenSource, Process) has cleanup in both success and error paths. Pending accepts are neutralized. ServerCancellation is only disposed when safe.
Defensive Coding: Callback exceptions are wrapped as InnerException in MtpServerConnectionClosedException. Null task from callback is caught. Cancel() runs on thread pool to avoid blocking registration.
Error Handling: Shared teardown task wrapped in catch-all so it never faults. ObserveFailure prevents UnobservedTaskException. Every teardown helper is non-throwing.
Tests: 68 unit tests + acceptance tests covering argument shape, lifecycle, faults, cancellation, timeouts, idempotent disposal, notification-handler disposal, and the real end-to-end in-process path.
Documentation: Changelog, protocol intro, PACKAGE.md all updated. XML doc comments are thorough.

The refactoring of shared transport logic into MtpServerConnector and the IMtpServerHost abstraction is clean and prevents the two launch paths from drifting. The existing LaunchAsync(path) path is provably unchanged (same argument string, same failure messages, same teardown order).

Copilot

Copilot review overview

Review tier: Balanced
Findings: 3

New issues introduced by this change (3)

Severity	Finding
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `MtpJsonRpcConnection.Dispose()` closes the socket and then waits up to its 5-second read-loop…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `ShutdownAsync` writes this nullable property from the teardown worker while callers can read…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — A canceled callback takes this branch without an inner exception, although the package…

What changed in this PR

Adds in-process launch support to the source-only MTP server-mode client for embedded/mobile hosts, addressing #10890 and enabling scenarios related to #9809.

Changes:

Adds callback-based in-process hosting with shared transport setup.
Adds bounded asynchronous shutdown and server exit-code reporting.
Adds documentation, unit coverage, and real-MSTest acceptance coverage.

File	Description
`MtpServerClientInProcessTests.cs`	Tests in-process lifecycle and protocol behavior.
`FakeMtpServer.cs`	Supports server-to-client dial-back mode.
`MtpServerClientInProcessAcceptanceTests.cs`	Exercises a real in-process MSTest application.
`MtpServerClientAcceptanceTests.cs`	Covers external-process shutdown and exit code.
`MtpServerClientSourcePackageConsumerTests.cs`	Extends source-package compile coverage.
`PACKAGE.md`	Documents embedded-host usage and lifecycle.
`MtpServerProcess.cs`	Adopts shared host and shutdown abstractions.
`MtpServerInProcessHost.cs`	Implements callback hosting and teardown.
`MtpServerConnector.cs`	Centralizes listener and transport setup.
`MtpServerClientOptions.cs`	Adds the shutdown timeout option.
`MtpServerClientExceptions.cs`	Supports preserved inner exceptions.
`MtpServerClient.cs`	Exposes in-process launch and shutdown.
`IMtpServerHost.cs`	Defines common host ownership behavior.
`IMtpServerClient.cs`	Adds shutdown and exit-code members.
`001-protocol-intro.md`	Documents reference-client launch modes.
`Changelog-Platform.md`	Records the new embedded-host capability.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+ Connection.Dispose();
+ SafeDispose(_client, _logger, "Disposing the accepted client socket");
+ MtpServerConnector.SafeStop(_listener, _logger);
+
+ bool stopped = await ShutdownServerAsync(_serverTask, _serverCancellation, _shutdownTimeout, _logger).ConfigureAwait(false);


+ /// Gets the exit code the hosted application returned, or <see langword="null"/> while it is still
+ /// running (or when it failed or was abandoned rather than returning one).
+ /// </summary>
+ public int? ExitCode { get; private set; }


+ if (serverTask.IsCanceled)
+ {
+ return new MtpServerConnectionClosedException(
+ "The in-process Microsoft.Testing.Platform application was canceled before connecting back.");
+ }


Amaury Levé (Evangelink) · 2026-08-31T19:22:25Z

Fixes#10890

Why

MtpServerClient.LaunchAsync(path) owns the loopback listener and starts the test application with Process.Start. That is right for IDE and desktop tooling, but unusable for embedded hosts — MAUI apps, Android/iOS test apps — where the MTP application already runs in the caller's process.

Today those hosts have to reimplement the listener, the --server/host/port arguments, the race between connection and startup failure, the serializer-before-formatter registration order, TcpMessageHandler + MtpJsonRpcConnection construction, and the exit / transport-close / server-completion shutdown dance. That defeats the point of shipping a canonical client and makes it easy to reintroduce bugs this package already solved: partial frame writes, ignored JSON-RPC errors, missing $/cancelRequest, unbounded waits, exception masking.

A concrete consumer is the DeviceRunners MSTest visual-runner work (mattleibow/DeviceRunners#157, #9809).

What

usingIMtpServerClientclient=awaitMtpServerClient.LaunchInProcessAsync(async(serverArgs,token)=>{ITestApplicationBuilderbuilder=awaitTestApplication.CreateBuilderAsync(serverArgs);builder.AddMSTest(()=>testAssemblies);usingITestApplicationapp=awaitbuilder.BuildAsync();returnawaitapp.RunAsync();},options,cancellationToken);awaitclient.InitializeAsync();awaitclient.DiscoverTestsAsync();awaitclient.RunTestsAsync();awaitclient.ExitAsync();awaitclient.ShutdownAsync();

The caller supplies only "how to run the application". The client keeps everything else: it binds the listener, generates the complete argument array (--server jsonrpc --client-host 127.0.0.1 --client-port <port> --no-banner), races the connect, builds the transport, and owns a bounded shutdown.

New API on the injected (still internal) surface:

Member	Purpose
`MtpServerClient.LaunchInProcessAsync(callback, options, ct)`	The embedded-host launch path. Async only — blocking the launching thread can deadlock the application being launched.
`IMtpServerClient.ShutdownAsync()`	Non-blocking teardown.
`IMtpServerClient.ServerExitCode`	The value the application returned.
`MtpServerClientOptions.ServerShutdownTimeout`	Graceful shutdown bound (default 30s).

Internally, the transport setup was extracted from MtpServerProcess into MtpServerConnector, and both launch paths now sit behind IMtpServerHost, so they cannot drift.

Behavior worth reviewing

Ownership and shutdown. Both Dispose() and ShutdownAsync() join one lazily created shared teardown task, on both hosts. A Dispose that follows or races ShutdownAsync therefore returns only once the server has actually stopped, rather than reporting success while teardown is still running. Teardown runs on the thread pool, so ShutdownAsync never blocks and the synchronous Dispose cannot deadlock against a UI-thread continuation. Task.Run captures the execution context, so the connection's read-loop AsyncLocal marker still flows and disposing from a notification handler does not self-wait (covered by a test asserting < 4s against the connection's 5s read-loop timeout).

Bounded, and actually bounded. Teardown waits ServerShutdownTimeout, then cancels the callback's token, then a fixed 5s grace, then abandons and logs. CancellationTokenSource.Cancel() runs registrations synchronously, so the cancellation is started separately — otherwise a blocking caller registration would prevent the grace from ever starting and make the "bounded" wait unbounded. A failed launch skips the graceful wait entirely: nothing is connected, so there is no transport closure for the callback to observe.

Exception preservation. A callback that throws, is canceled, or returns before dialing back surfaces as MtpServerConnectionClosedException with the original as InnerException, instead of a misleading connection timeout. Every teardown helper is non-throwing, and the shared teardown task is wrapped so it can never fault — a faulted shared task would throw from every later disposal.

LaunchAsync(path) is unchanged. Verified against main: identical argument string, failure messages, stderr capture, exit fast-fail and teardown order. One earlier revision of this branch added a grace period to the accept race; review showed it only traded a precise failure for a vague one, so it was removed and the external path is now provably unchanged.

Browser/WASM. Both paths are loopback TCP. LaunchInProcessAsync fails fast with PlatformNotSupportedException there; this does not enable WASM hosting, and the docs say so.

Tests

68 unit tests (net8.0) / 63 (net462, Jsonite path). New coverage: argument array shape; initialize → discover → run → exit; callback faulting synchronously and asynchronously; callback exiting with a code; null task; pre-canceled launch not invoking the callback; cancellation during connect canceling the callback token; connection timeout bounded by the grace rather than ServerShutdownTimeout; ShutdownAsync; ServerExitCode; disposal awaiting the callback; Dispose racing an in-flight ShutdownAsync; Dispose from a notification handler; idempotent disposal; callback faulting during shutdown; unresponsive callback abandoned within the bound; $/cancelRequest; stateful on/off; multi-request single connection; EnvironmentVariables ignored and warned. Stressed 6×/5× consecutively for flakiness.
New acceptance test where a single generated process is simultaneously the embedded host (compiling the packed source-only package) and a real MTP TestApplication with real MSTest over a real [TestClass] — discovering and running its own test over JSON-RPC with no Process.Start anywhere. It also asserts the server's reported process id equals its own.
Existing external-process acceptance test extended to cover ShutdownAsync + ServerExitCode; hostile-consumer compile oracle extended to bind the new API on net462/netstandard2.0/net5.0–net8.0.
Full regression: 13 platform source-package/consumer acceptance, 4 MSTest acceptance, 13 platform ServerTests. build.cmd -pack clean, 0 warnings.

Review history

Four independent review rounds (MTP/MSTest expert reviewer, a design reviewer, and two correctness reviewers) produced 3 major, 10 moderate and 10 minor findings, all addressed. Several were real bugs the tests then locked in — notably Process.ExitCode being unreadable after Process.Dispose(), a TcpListener socket leak when Start() failed, and a teardown path that could throw from a contract documented never to throw.

Open design questions

No IAsyncDisposable. netstandard2.0/net462 would need Microsoft.Bcl.AsyncInterfaces, breaking the package's dependency-free promise. ShutdownAsync() is the substitute. Happy to revisit if the dependency is acceptable.
MtpServerClientOptions is mode-mixed.EnvironmentVariables is external-process only; ServerShutdownTimeout is in-process only. Nesting per-transport options would age better, but EnvironmentVariables already shipped at the top level, so it cannot be done non-breakingly now.
The 5s cancellation grace is a fixed constant, not an option.
The two paths still build different argument shapes — deliberate, to keep the shipped external command line byte-identical. Commented at the call site.
No ConnectAsync(TcpClient/Stream, options) factory — the existing MtpServerClient(MtpJsonRpcConnection, options) constructor already covers "wrap an existing transport", and the guidance is to minimize injected surface. Easy to add if wanted.

Embedded hosts such as MAUI or Android/iOS test apps cannot spawn a child process, so `MtpServerClient.LaunchAsync(path)` was unusable for them and they had to reimplement the listener, the server-mode arguments, the connect race, the serializer/formatter ordering, the transport and the shutdown coordination by hand -- reintroducing the bugs the canonical client already solves. Add `MtpServerClient.LaunchInProcessAsync(callback, options, cancellationToken)`. The client keeps ownership of everything except "how to run the application": it binds the loopback listener, generates the complete server-mode argument array, races the connect against callback failure/completion, caller cancellation and the connection timeout, and owns a bounded shutdown that closes the transport, then cancels the callback token, then abandons it rather than hanging the caller. A callback failure before connection is surfaced as `MtpServerConnectionClosedException` with the caller's exception preserved as the inner exception, and teardown failures are only logged so they can never replace the primary failure. The shared transport setup is extracted from `MtpServerProcess` into `MtpServerConnector` and both launch paths now flow through `IMtpServerHost`, so the external-process behavior is unchanged while the two paths cannot drift. The one behavior change is an improvement shared by both: when the server is seen to have stopped, a still-pending accept gets a bounded final grace so a connection established just before the stop is not discarded in favor of a misleading "stopped before connecting back" failure. The path is loopback TCP, so it fails fast with `PlatformNotSupportedException` on browser/WASM; it does not enable WASM hosting. Fixes#10890 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Two independent reviews of the in-process launch path raised the same top finding: `Dispose()` waits for the hosted application synchronously on the calling thread, and on the very platforms this feature targets (MAUI, Android, iOS) a multi-second block on the UI thread trips the ANR / watchdog. Add `IMtpServerClient.ShutdownAsync()` (and `IMtpServerHost.ShutdownAsync`) so the same teardown can be awaited instead. `IAsyncDisposable` stays rejected -- netstandard2.0 and net462 would need `Microsoft.Bcl.AsyncInterfaces`, which breaks the package's dependency-free promise -- but rejecting the interface is not a reason to have no asynchronous shutdown at all. `Dispose()` remains, is still idempotent, and returns immediately after `ShutdownAsync`. Remove the accept grace period. The previous commit let a still-pending accept win for ~100ms after the server was seen to have stopped, on the theory that a connection established just before the stop should not be discarded. That theory does not hold: if the server has stopped, the socket belongs to a dead peer, so the grace only traded a precise failure (exit code plus captured stderr) for a generic connection-closed error on the first request, and its uncancellable delay could let the server failure beat a concurrent caller cancellation. Dropping it also makes the external-process path provably unchanged, so the changelog no longer needs a `Changed` entry. Other review fixes: * Expose `IMtpServerClient.ServerExitCode`. The callback signature already demanded a `Task<int>`, but after a successful session the value was unreachable, so an embedded host whose `Main` must return it had to capture it in a closure. * Do not dispose the server's `CancellationTokenSource` when the callback was abandoned while still running: it holds the token, and `token.WaitHandle` or `CreateLinkedTokenSource` would then throw `ObjectDisposedException` inside the caller's own code. * Skip the graceful wait entirely on a failed launch. Nothing is connected, so there is no transport closure for the callback to observe; only the fixed cancellation grace applies and an unwinding caller no longer pays `ServerShutdownTimeout`. * Clamp bounded waits instead of trusting the caller's `TimeSpan`. A negative value (or `Timeout.InfiniteTimeSpan`) made `Task.Delay` throw from a path documented never to throw; an oversized one is capped to the largest delay .NET Framework accepts. * Move `TcpListener.Start()` inside the cleanup `try`: it creates the socket before binding, so a bind failure leaked it because the caller never received a listener it could stop. * Log a late failure from an abandoned callback rather than only observing it. * Cache the in-process host's `ProcessId` instead of allocating a `Process` per property read. * Document that `Dispose()` blocks, that cancellation is bounded rather than immediate, and why the two launch paths still build different argument shapes; fix the shadowed `cancellationToken` in the PACKAGE.md sample. Tests: cover `ShutdownAsync`, `ServerExitCode`, and fail-fast on a server that stops without connecting; make `Dispose_IsIdempotent` able to fail by asserting on observed transport closes rather than callback invocations; replace the hand-rolled throws helper with `Assert.ThrowsExactlyAsync`; tighten the shutdown-bound assertion; and stop leaking `Process` handles. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

A third review round found three ways the teardown contract did not hold up under concurrency. `Dispose()` and `ShutdownAsync()` each had their own `Interlocked.Exchange` guard, so whichever call lost the race returned while teardown was still running -- reporting to its caller that the application had stopped when it had not. Replace both guards with one lazily started, shared teardown task: every entry point now joins the same task, so `Dispose` after (or racing) `ShutdownAsync` returns only once the application has actually stopped, and the whole thing stays idempotent. `MtpServerClient` no longer keeps its own guard either; it delegates to the host, whose teardown is the single source of truth. `ShutdownAsync()` could still block the very thread it exists to protect: it closed the transport before its first await, and closing the connection waits up to five seconds for the read loop. The shared task is started with `Task.Run`, so the whole teardown -- transport close included -- runs on the thread pool and `ShutdownAsync` returns immediately. `MtpServerProcess` likewise now runs its (bounded but synchronous) kill off the calling thread rather than pretending to be async while blocking. `CancellationTokenSource.Cancel()` executes registrations synchronously on the calling thread, so a caller registration that blocked would prevent the five-second cancellation grace from ever starting and make the "bounded" shutdown unbounded. Start the cancellation separately and begin the grace regardless. When a registration is still executing once the callback has finished, the source is reported as unsafe to dispose for the same reason an abandoned callback is: leaking one `CancellationTokenSource` beats a use-after-dispose inside the caller's code. Adds a test that a `Dispose` racing an in-flight `ShutdownAsync` blocks until the shared teardown completes, and that both share one teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

… fix ExitCode A fourth review round found that only the in-process host had been converted to the shared-teardown design. `MtpServerProcess` kept its old early-return guard, so on the external-process path a `Dispose()` that followed or raced `ShutdownAsync()` returned while `SafeKill`'s bounded `WaitForExit` was still running on another thread -- losing exactly the guarantee that wait exists for (a caller may delete the application directory immediately after disposal), and making `await ShutdownAsync()` report that the server had stopped when it had not. That contradicted the comment the previous commit added to `MtpServerClient.Dispose`, which claimed both paths joined an in-flight teardown. Convert `MtpServerProcess` to the same lazily created shared task, so the contract is now uniform across both implementations of `IMtpServerHost`, and reconcile the interface docs, which previously described the opposite rule. Adding real coverage for `ShutdownAsync` on the external-process path then surfaced a genuine bug: `MtpServerProcess.ExitCode` was always `null` after teardown, because a `Process` cannot be queried once disposed. Capture the exit code during teardown instead -- before the kill, so an application that already exited on its own reports its real code rather than the kill's, and before `Process.Dispose()`, after which nothing is readable. Also fixes a race in the in-process test fixture that made the suite flaky on net462: the callback published its `FakeMtpServer` only after its own connect call returned, but the client's accept can complete first, so a test could reach `Value` before the callback had set it. The fixture now exposes a `Connected` signal the launch helper awaits. 6/6 clean net462 runs and 5/5 clean net8.0 runs afterwards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

+ if (serverTask is not null)
+ {
+ // The launch is being abandoned, so skip the graceful wait entirely: there is no connected
+ // transport whose closure could signal the callback, and the caller (often a canceling one) is
+ // waiting on this unwind. A zero graceful timeout goes straight to cancel-then-grace.
+ if (!await ShutdownServerAsync(serverTask, serverCancellation!, TimeSpan.Zero, logger).ConfigureAwait(false))
+ {
+ // The callback is still running and still holds the token; disposing its source now would
+ // turn a clean abandonment into an ObjectDisposedException inside the caller's own code.
+ throw;
+ }
+ }


github-actions · 2026-08-31T19:32:23Z

🧪 Expert test review — PR #10898

Grade	Test	Mutation	Notes	How to improve
B (80–89)	new `MtpServerClientInProcessTests. Dispose_CallbackFaultsDuringShutdown_ DoesNotThrow`	2/3 killed	Reads `server.Completion.Exception` but never asserts on it, so a mutation changing the faulted exception's identity/message survives.	Assert the captured exception's message/type instead of discarding it after touching `.Exception`.
B (80–89)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_HonorsTheStatefulOption`	3/4 killed	Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on `client.Capabilities` is not caught.	Also assert `client.Capabilities.IsStateful` reflects the negotiated round-trip value, not only the sent request.
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_PassesCompleteServerModeArguments`	5/5 killed	Verifies exact ordered arguments, dynamic port and fixed count; strong regression guard for the generated argument array.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_DrivesInitializeDiscoverRunAndExit`	6/6 killed	End-to-end drive through initialize/discover/run/exit with distinct assertions per state transition.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsBeforeConnecting_ PreservesCallbackException`	2/2 killed	Asserts the exact exception instance is preserved as inner exception, not just its type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsAsynchronouslyBeforeConnecting_ PreservesCallbackException`	2/2 killed	Covers the async-throw variant with the same identity assertion as the sync case.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsWithoutConnecting_ FailsFastInsteadOfWaitingOutTheTimeout`	3/3 killed	Checks both the reported exit code in the message and a timing bound guarding against a slow-timeout regression.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsBeforeConnecting_ ReportsExitCode`	2/2 killed	Asserts the specific exit code surfaces in the exception message.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackReturnsNullTask_Fails`	2/2 killed	Asserts the specific inner exception type for a misbehaving callback returning a null task.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_NullCallback_Throws`	1/1 killed	Focused single-assertion guard-clause test using the exact exception type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_AlreadyCanceled_ DoesNotInvokeCallback`	2/2 killed	Verifies both the cancellation exception and, via an interlocked counter, that the callback never ran.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CanceledWhileConnecting_ CancelsTheCallbackToken`	2/2 killed	Confirms both the caller-side cancellation and that the callback's own token observed cancellation.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_ConnectionTimeoutElapses_ FailsWithTimeoutMessage`	3/3 killed	Distinguishes the connection timeout from the deliberately huge shutdown timeout via message and elapsed-time bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_ClosesTransportAndAwaitsTheCallback`	4/4 killed	Checks pre/post completion state, the callback's returned exit code, and the client's reported exit code.	—
A (90–100)	new `MtpServerClientInProcessTests. ShutdownAsync_ClosesTransportAndAwaitsTheCallback_ WithoutBlocking`	4/4 killed	Verifies await-completion, exit code, a fast follow-up Dispose, and single-teardown count together.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_FromANotificationHandler_ DoesNotSelfWaitOnTheReadLoop`	1/1 killed	Targeted timing assertion guards against a specific re-entrancy deadlock regression.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_WhileShutdownAsyncIsInFlight_ WaitsForTheSameTeardown`	3/3 killed	Uses a controlled release gate to prove the racing Dispose genuinely blocks, then confirms a single shared teardown.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_IsIdempotent`	3/3 killed	Confirms exactly one transport close and one completion despite three Dispose calls, plus a fast-return bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_CallbackIgnoresShutdown_ ReturnsWithinTheDocumentedBound`	2/2 killed	Verifies both the documented abandonment time bound and the logged "abandoning" diagnostic.	—
A (90–100)	new `MtpServerClientInProcessTests. RunTestsAsync_Canceled_ SendsCancelRequestToTheHostedApplication`	2/2 killed	Confirms both the client-side cancellation exception and the wire-level cancel notification reaching the fake server.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_MultipleRequestsOnOneSession_ ReuseTheSameConnection`	3/3 killed	Checks keep-alive negotiation, single-connection reuse count, and total request count together.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_IgnoresEnvironmentVariablesAndWarns`	1/1 killed	Asserts the specific warning content naming the ignored option.	—
A (90–100)	new `MtpServerClientInProcessAcceptanceTests. InProcessHost_ DiscoversAndRunsItsOwnMSTestNodes_ WithoutStartingAProcess`	5/5 killed	Real end-to-end embedded-host run asserting build success, exit code, and each marker line the generated app emits.	—
A (90–100)	mod `MtpServerClientAcceptanceTests. DiscoverAndRun_ViaSourcePackageClient_ ReportsExpectedTestNode`	1/1 killed	New lines assert the external-process ShutdownAsync/Dispose share a teardown and report an exit code.	—
A (90–100)	mod `MtpServerClientSourcePackageConsumerTests. HostileConsumer_ CompilesAgainstPackedSource`	N/A	Compile-only guard; added lines correctly extend surface coverage to `ShutdownAsync` and the in-process launch path.	—

_{This advisory comment was generated automatically. Grades are heuristic

and informational — they do not block merging. Suggestions on the Files

changed tab can be applied with one click. Re-run with
/review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · [◷]( · ◷)

github-actions · 2026-08-31T19:32:24Z

🧵 Parallel-safety audit — PR #10898

Parallelization — audited assemblies:

Test assembly	Scope	Workers	Analyzer coverage
`MSTest.Acceptance.IntegrationTests`	`MethodLevel` (default `[assembly: Parallelize(Scope = ExecutionScope.MethodLevel, Workers = 0)]` in `Program.cs`)	CPU count	coverable once the parallel-safety analyzers ship (attribute-based opt-in)
`Microsoft.Testing.Platform.Acceptance.IntegrationTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped
`Microsoft.Testing.Platform.ServerMode.Client.Sources.UnitTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped

This PR did not touch any .runsettings/testconfig.json/Directory.Build.* parallelization config — no assembly's scope changed.

Findings: A (global-state) 0 · B (paths) 0 · C (declaration) 0 · D (over-serialization) 0 — by severity: Critical 0 · High 0 · Warning 0 · Info 0.

No unsafe call sites found in the lines this PR added or modified.

MtpServerClientAcceptanceTests.cs — added lines only call client.ShutdownAsync() / assert ServerExitCode; no process-global state touched.
MtpServerClientSourcePackageConsumerTests.cs — added lines call into an isolated generated test asset (DriveAsync/DriveInProcessAsync); no shared/relative paths, env vars, culture, or console state mutated at the assembly level (Console.WriteLine here writes to the generated app's own redirected output, not the auditing process).
MtpServerClientInProcessAcceptanceTests.cs (new) — uses TestAsset.GenerateAssetAsync, which allocates a unique GUID-suffixed TempDirectory per instance (TestAsset.cs:19-26, throws if a path already exists) — no shared-path collision. CreateChildEnvironment() builds a fresh Dictionary passed only to the spawned child process's environment, not Environment.SetEnvironmentVariable, so it does not mutate this process's state.
FakeMtpServer.cs / MtpServerClientInProcessTests.cs (new) — each test constructs its own TcpListener/FakeMtpServer bound to an OS-assigned loopback port (new TcpListener(IPAddress.Loopback, 0)), so there is no fixed-port collision between concurrently-running tests; no static/process-global state is written.

Nothing here implies a testability or smell concern beyond this audit's scope.

_{Advisory only — heuristic, non-blocking. Re-run with /parallel-audit. This audit answers "is it parallel-safe?"; for testability, smells, or flakiness see the detect-static-dependencies / test-smell-detection / test-anti-patterns analyses.}

🤖 Automated content by GitHub Copilot. Generated by the Parallel-safety audit on PR (on open / sync) workflow. · auto · 117.1 AIC · ⌖ 3.53 AIC · ⊞ 24.8K · [◷]( · ◷)

github-actions

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · ◷

github-actions · 2026-08-31T19:32:25Z

+ client.Dispose();
+
+ Assert.IsTrue(server.Completion.IsFaulted);
+ _ = server.Completion.Exception;


🧪 Test review · Grade B (80–89) — Reads server.Completion.Exception but never asserts on it, so a mutation changing the faulted exception's identity/message would survive.
Assert the captured exception's message/type instead of discarding it after touching .Exception.
Suggested change
_=server.Completion.Exception;
Exception?faultException=server.Completion.Exception?.GetBaseException();
Assert.IsInstanceOfType<InvalidOperationException>(faultException);
Assert.AreEqual("The application failed while shutting down.",faultException!.Message);

github-actions · 2026-08-31T19:32:25Z

+ ? typed
+ : SerializerUtilities.Deserialize<InitializeRequestArgs>((IDictionary<string, object?>)initialize.Params!);
+
+ Assert.AreEqual(isStateful, args.Capabilities.IsStateful, "The in-process path must forward the client's stateful capability unchanged.");


🧪 Test review · Grade B (80–89) — Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on client.Capabilities is not caught.
Also assert client.Capabilities.IsStateful reflects the negotiated round-trip value, not only the sent request.
Suggested change
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,capabilities.IsStateful,"The negotiated capability the client stores must reflect the server's round trip, not just the sent request.");

github-actions

Note

🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

✅ 22/22 dimensions clean — no findings.

Summary: This PR adds MtpServerClient.LaunchInProcessAsync for embedded hosts (MAUI, Android/iOS) that cannot Process.Start. The implementation is thorough and well-structured:

Threading & Concurrency: Shutdown is idempotent via lock + lazy Task, Volatile.Read/Write for cross-thread exit code, Task.Run avoids sync-context deadlocks. No shared mutable state without synchronization.
Security & IPC: Loopback TCP only, no command-line injection surface (arguments are array-based), WASM fails fast with PlatformNotSupportedException.
Public API: All new API is internal (source-only package). No init accessors. No PublicAPI.Unshipped.txt changes needed.
Performance: GetCurrentProcessId() snapshotted once in constructor. No hot-path allocations. WaitBoundedAsync clamps timeouts correctly.
Cross-TFM: #if NET8_0_OR_GREATER guard on AcceptTcpClientAsync(CancellationToken). RuntimeInformation used for OS detection on net462.
Resource Management: Every disposable (TcpListener, TcpClient, CancellationTokenSource, Process) has cleanup in both success and error paths. Pending accepts are neutralized. ServerCancellation is only disposed when safe.
Defensive Coding: Callback exceptions are wrapped as InnerException in MtpServerConnectionClosedException. Null task from callback is caught. Cancel() runs on thread pool to avoid blocking registration.
Error Handling: Shared teardown task wrapped in catch-all so it never faults. ObserveFailure prevents UnobservedTaskException. Every teardown helper is non-throwing.
Tests: 68 unit tests + acceptance tests covering argument shape, lifecycle, faults, cancellation, timeouts, idempotent disposal, notification-handler disposal, and the real end-to-end in-process path.
Documentation: Changelog, protocol intro, PACKAGE.md all updated. XML doc comments are thorough.

The refactoring of shared transport logic into MtpServerConnector and the IMtpServerHost abstraction is clean and prevents the two launch paths from drifting. The existing LaunchAsync(path) path is provably unchanged (same argument string, same failure messages, same teardown order).

Copilot

Copilot review overview

Review tier: Balanced
Findings: 3

New issues introduced by this change (3)

Severity	Finding
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `MtpJsonRpcConnection.Dispose()` closes the socket and then waits up to its 5-second read-loop…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `ShutdownAsync` writes this nullable property from the teardown worker while callers can read…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — A canceled callback takes this branch without an inner exception, although the package…

What changed in this PR

Adds in-process launch support to the source-only MTP server-mode client for embedded/mobile hosts, addressing #10890 and enabling scenarios related to #9809.

Changes:

Adds callback-based in-process hosting with shared transport setup.
Adds bounded asynchronous shutdown and server exit-code reporting.
Adds documentation, unit coverage, and real-MSTest acceptance coverage.

File	Description
`MtpServerClientInProcessTests.cs`	Tests in-process lifecycle and protocol behavior.
`FakeMtpServer.cs`	Supports server-to-client dial-back mode.
`MtpServerClientInProcessAcceptanceTests.cs`	Exercises a real in-process MSTest application.
`MtpServerClientAcceptanceTests.cs`	Covers external-process shutdown and exit code.
`MtpServerClientSourcePackageConsumerTests.cs`	Extends source-package compile coverage.
`PACKAGE.md`	Documents embedded-host usage and lifecycle.
`MtpServerProcess.cs`	Adopts shared host and shutdown abstractions.
`MtpServerInProcessHost.cs`	Implements callback hosting and teardown.
`MtpServerConnector.cs`	Centralizes listener and transport setup.
`MtpServerClientOptions.cs`	Adds the shutdown timeout option.
`MtpServerClientExceptions.cs`	Supports preserved inner exceptions.
`MtpServerClient.cs`	Exposes in-process launch and shutdown.
`IMtpServerHost.cs`	Defines common host ownership behavior.
`IMtpServerClient.cs`	Adds shutdown and exit-code members.
`001-protocol-intro.md`	Documents reference-client launch modes.
`Changelog-Platform.md`	Records the new embedded-host capability.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+ Connection.Dispose();
+ SafeDispose(_client, _logger, "Disposing the accepted client socket");
+ MtpServerConnector.SafeStop(_listener, _logger);
+
+ bool stopped = await ShutdownServerAsync(_serverTask, _serverCancellation, _shutdownTimeout, _logger).ConfigureAwait(false);


+ /// Gets the exit code the hosted application returned, or <see langword="null"/> while it is still
+ /// running (or when it failed or was abandoned rather than returning one).
+ /// </summary>
+ public int? ExitCode { get; private set; }


+ if (serverTask.IsCanceled)
+ {
+ return new MtpServerConnectionClosedException(
+ "The in-process Microsoft.Testing.Platform application was canceled before connecting back.");
+ }


Amaury Levé (Evangelink) · 2026-08-31T19:22:25Z

Fixes#10890

Why

MtpServerClient.LaunchAsync(path) owns the loopback listener and starts the test application with Process.Start. That is right for IDE and desktop tooling, but unusable for embedded hosts — MAUI apps, Android/iOS test apps — where the MTP application already runs in the caller's process.

Today those hosts have to reimplement the listener, the --server/host/port arguments, the race between connection and startup failure, the serializer-before-formatter registration order, TcpMessageHandler + MtpJsonRpcConnection construction, and the exit / transport-close / server-completion shutdown dance. That defeats the point of shipping a canonical client and makes it easy to reintroduce bugs this package already solved: partial frame writes, ignored JSON-RPC errors, missing $/cancelRequest, unbounded waits, exception masking.

A concrete consumer is the DeviceRunners MSTest visual-runner work (mattleibow/DeviceRunners#157, #9809).

What

usingIMtpServerClientclient=awaitMtpServerClient.LaunchInProcessAsync(async(serverArgs,token)=>{ITestApplicationBuilderbuilder=awaitTestApplication.CreateBuilderAsync(serverArgs);builder.AddMSTest(()=>testAssemblies);usingITestApplicationapp=awaitbuilder.BuildAsync();returnawaitapp.RunAsync();},options,cancellationToken);awaitclient.InitializeAsync();awaitclient.DiscoverTestsAsync();awaitclient.RunTestsAsync();awaitclient.ExitAsync();awaitclient.ShutdownAsync();

The caller supplies only "how to run the application". The client keeps everything else: it binds the listener, generates the complete argument array (--server jsonrpc --client-host 127.0.0.1 --client-port <port> --no-banner), races the connect, builds the transport, and owns a bounded shutdown.

New API on the injected (still internal) surface:

Member	Purpose
`MtpServerClient.LaunchInProcessAsync(callback, options, ct)`	The embedded-host launch path. Async only — blocking the launching thread can deadlock the application being launched.
`IMtpServerClient.ShutdownAsync()`	Non-blocking teardown.
`IMtpServerClient.ServerExitCode`	The value the application returned.
`MtpServerClientOptions.ServerShutdownTimeout`	Graceful shutdown bound (default 30s).

Internally, the transport setup was extracted from MtpServerProcess into MtpServerConnector, and both launch paths now sit behind IMtpServerHost, so they cannot drift.

Behavior worth reviewing

Ownership and shutdown. Both Dispose() and ShutdownAsync() join one lazily created shared teardown task, on both hosts. A Dispose that follows or races ShutdownAsync therefore returns only once the server has actually stopped, rather than reporting success while teardown is still running. Teardown runs on the thread pool, so ShutdownAsync never blocks and the synchronous Dispose cannot deadlock against a UI-thread continuation. Task.Run captures the execution context, so the connection's read-loop AsyncLocal marker still flows and disposing from a notification handler does not self-wait (covered by a test asserting < 4s against the connection's 5s read-loop timeout).

Bounded, and actually bounded. Teardown waits ServerShutdownTimeout, then cancels the callback's token, then a fixed 5s grace, then abandons and logs. CancellationTokenSource.Cancel() runs registrations synchronously, so the cancellation is started separately — otherwise a blocking caller registration would prevent the grace from ever starting and make the "bounded" wait unbounded. A failed launch skips the graceful wait entirely: nothing is connected, so there is no transport closure for the callback to observe.

Exception preservation. A callback that throws, is canceled, or returns before dialing back surfaces as MtpServerConnectionClosedException with the original as InnerException, instead of a misleading connection timeout. Every teardown helper is non-throwing, and the shared teardown task is wrapped so it can never fault — a faulted shared task would throw from every later disposal.

LaunchAsync(path) is unchanged. Verified against main: identical argument string, failure messages, stderr capture, exit fast-fail and teardown order. One earlier revision of this branch added a grace period to the accept race; review showed it only traded a precise failure for a vague one, so it was removed and the external path is now provably unchanged.

Browser/WASM. Both paths are loopback TCP. LaunchInProcessAsync fails fast with PlatformNotSupportedException there; this does not enable WASM hosting, and the docs say so.

Tests

68 unit tests (net8.0) / 63 (net462, Jsonite path). New coverage: argument array shape; initialize → discover → run → exit; callback faulting synchronously and asynchronously; callback exiting with a code; null task; pre-canceled launch not invoking the callback; cancellation during connect canceling the callback token; connection timeout bounded by the grace rather than ServerShutdownTimeout; ShutdownAsync; ServerExitCode; disposal awaiting the callback; Dispose racing an in-flight ShutdownAsync; Dispose from a notification handler; idempotent disposal; callback faulting during shutdown; unresponsive callback abandoned within the bound; $/cancelRequest; stateful on/off; multi-request single connection; EnvironmentVariables ignored and warned. Stressed 6×/5× consecutively for flakiness.
New acceptance test where a single generated process is simultaneously the embedded host (compiling the packed source-only package) and a real MTP TestApplication with real MSTest over a real [TestClass] — discovering and running its own test over JSON-RPC with no Process.Start anywhere. It also asserts the server's reported process id equals its own.
Existing external-process acceptance test extended to cover ShutdownAsync + ServerExitCode; hostile-consumer compile oracle extended to bind the new API on net462/netstandard2.0/net5.0–net8.0.
Full regression: 13 platform source-package/consumer acceptance, 4 MSTest acceptance, 13 platform ServerTests. build.cmd -pack clean, 0 warnings.

Review history

Four independent review rounds (MTP/MSTest expert reviewer, a design reviewer, and two correctness reviewers) produced 3 major, 10 moderate and 10 minor findings, all addressed. Several were real bugs the tests then locked in — notably Process.ExitCode being unreadable after Process.Dispose(), a TcpListener socket leak when Start() failed, and a teardown path that could throw from a contract documented never to throw.

Open design questions

No IAsyncDisposable. netstandard2.0/net462 would need Microsoft.Bcl.AsyncInterfaces, breaking the package's dependency-free promise. ShutdownAsync() is the substitute. Happy to revisit if the dependency is acceptable.
MtpServerClientOptions is mode-mixed.EnvironmentVariables is external-process only; ServerShutdownTimeout is in-process only. Nesting per-transport options would age better, but EnvironmentVariables already shipped at the top level, so it cannot be done non-breakingly now.
The 5s cancellation grace is a fixed constant, not an option.
The two paths still build different argument shapes — deliberate, to keep the shipped external command line byte-identical. Commented at the call site.
No ConnectAsync(TcpClient/Stream, options) factory — the existing MtpServerClient(MtpJsonRpcConnection, options) constructor already covers "wrap an existing transport", and the guidance is to minimize injected surface. Easy to add if wanted.

Embedded hosts such as MAUI or Android/iOS test apps cannot spawn a child process, so `MtpServerClient.LaunchAsync(path)` was unusable for them and they had to reimplement the listener, the server-mode arguments, the connect race, the serializer/formatter ordering, the transport and the shutdown coordination by hand -- reintroducing the bugs the canonical client already solves. Add `MtpServerClient.LaunchInProcessAsync(callback, options, cancellationToken)`. The client keeps ownership of everything except "how to run the application": it binds the loopback listener, generates the complete server-mode argument array, races the connect against callback failure/completion, caller cancellation and the connection timeout, and owns a bounded shutdown that closes the transport, then cancels the callback token, then abandons it rather than hanging the caller. A callback failure before connection is surfaced as `MtpServerConnectionClosedException` with the caller's exception preserved as the inner exception, and teardown failures are only logged so they can never replace the primary failure. The shared transport setup is extracted from `MtpServerProcess` into `MtpServerConnector` and both launch paths now flow through `IMtpServerHost`, so the external-process behavior is unchanged while the two paths cannot drift. The one behavior change is an improvement shared by both: when the server is seen to have stopped, a still-pending accept gets a bounded final grace so a connection established just before the stop is not discarded in favor of a misleading "stopped before connecting back" failure. The path is loopback TCP, so it fails fast with `PlatformNotSupportedException` on browser/WASM; it does not enable WASM hosting. Fixes#10890 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Two independent reviews of the in-process launch path raised the same top finding: `Dispose()` waits for the hosted application synchronously on the calling thread, and on the very platforms this feature targets (MAUI, Android, iOS) a multi-second block on the UI thread trips the ANR / watchdog. Add `IMtpServerClient.ShutdownAsync()` (and `IMtpServerHost.ShutdownAsync`) so the same teardown can be awaited instead. `IAsyncDisposable` stays rejected -- netstandard2.0 and net462 would need `Microsoft.Bcl.AsyncInterfaces`, which breaks the package's dependency-free promise -- but rejecting the interface is not a reason to have no asynchronous shutdown at all. `Dispose()` remains, is still idempotent, and returns immediately after `ShutdownAsync`. Remove the accept grace period. The previous commit let a still-pending accept win for ~100ms after the server was seen to have stopped, on the theory that a connection established just before the stop should not be discarded. That theory does not hold: if the server has stopped, the socket belongs to a dead peer, so the grace only traded a precise failure (exit code plus captured stderr) for a generic connection-closed error on the first request, and its uncancellable delay could let the server failure beat a concurrent caller cancellation. Dropping it also makes the external-process path provably unchanged, so the changelog no longer needs a `Changed` entry. Other review fixes: * Expose `IMtpServerClient.ServerExitCode`. The callback signature already demanded a `Task<int>`, but after a successful session the value was unreachable, so an embedded host whose `Main` must return it had to capture it in a closure. * Do not dispose the server's `CancellationTokenSource` when the callback was abandoned while still running: it holds the token, and `token.WaitHandle` or `CreateLinkedTokenSource` would then throw `ObjectDisposedException` inside the caller's own code. * Skip the graceful wait entirely on a failed launch. Nothing is connected, so there is no transport closure for the callback to observe; only the fixed cancellation grace applies and an unwinding caller no longer pays `ServerShutdownTimeout`. * Clamp bounded waits instead of trusting the caller's `TimeSpan`. A negative value (or `Timeout.InfiniteTimeSpan`) made `Task.Delay` throw from a path documented never to throw; an oversized one is capped to the largest delay .NET Framework accepts. * Move `TcpListener.Start()` inside the cleanup `try`: it creates the socket before binding, so a bind failure leaked it because the caller never received a listener it could stop. * Log a late failure from an abandoned callback rather than only observing it. * Cache the in-process host's `ProcessId` instead of allocating a `Process` per property read. * Document that `Dispose()` blocks, that cancellation is bounded rather than immediate, and why the two launch paths still build different argument shapes; fix the shadowed `cancellationToken` in the PACKAGE.md sample. Tests: cover `ShutdownAsync`, `ServerExitCode`, and fail-fast on a server that stops without connecting; make `Dispose_IsIdempotent` able to fail by asserting on observed transport closes rather than callback invocations; replace the hand-rolled throws helper with `Assert.ThrowsExactlyAsync`; tighten the shutdown-bound assertion; and stop leaking `Process` handles. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

A third review round found three ways the teardown contract did not hold up under concurrency. `Dispose()` and `ShutdownAsync()` each had their own `Interlocked.Exchange` guard, so whichever call lost the race returned while teardown was still running -- reporting to its caller that the application had stopped when it had not. Replace both guards with one lazily started, shared teardown task: every entry point now joins the same task, so `Dispose` after (or racing) `ShutdownAsync` returns only once the application has actually stopped, and the whole thing stays idempotent. `MtpServerClient` no longer keeps its own guard either; it delegates to the host, whose teardown is the single source of truth. `ShutdownAsync()` could still block the very thread it exists to protect: it closed the transport before its first await, and closing the connection waits up to five seconds for the read loop. The shared task is started with `Task.Run`, so the whole teardown -- transport close included -- runs on the thread pool and `ShutdownAsync` returns immediately. `MtpServerProcess` likewise now runs its (bounded but synchronous) kill off the calling thread rather than pretending to be async while blocking. `CancellationTokenSource.Cancel()` executes registrations synchronously on the calling thread, so a caller registration that blocked would prevent the five-second cancellation grace from ever starting and make the "bounded" shutdown unbounded. Start the cancellation separately and begin the grace regardless. When a registration is still executing once the callback has finished, the source is reported as unsafe to dispose for the same reason an abandoned callback is: leaking one `CancellationTokenSource` beats a use-after-dispose inside the caller's code. Adds a test that a `Dispose` racing an in-flight `ShutdownAsync` blocks until the shared teardown completes, and that both share one teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

… fix ExitCode A fourth review round found that only the in-process host had been converted to the shared-teardown design. `MtpServerProcess` kept its old early-return guard, so on the external-process path a `Dispose()` that followed or raced `ShutdownAsync()` returned while `SafeKill`'s bounded `WaitForExit` was still running on another thread -- losing exactly the guarantee that wait exists for (a caller may delete the application directory immediately after disposal), and making `await ShutdownAsync()` report that the server had stopped when it had not. That contradicted the comment the previous commit added to `MtpServerClient.Dispose`, which claimed both paths joined an in-flight teardown. Convert `MtpServerProcess` to the same lazily created shared task, so the contract is now uniform across both implementations of `IMtpServerHost`, and reconcile the interface docs, which previously described the opposite rule. Adding real coverage for `ShutdownAsync` on the external-process path then surfaced a genuine bug: `MtpServerProcess.ExitCode` was always `null` after teardown, because a `Process` cannot be queried once disposed. Capture the exit code during teardown instead -- before the kill, so an application that already exited on its own reports its real code rather than the kill's, and before `Process.Dispose()`, after which nothing is readable. Also fixes a race in the in-process test fixture that made the suite flaky on net462: the callback published its `FakeMtpServer` only after its own connect call returned, but the client's accept can complete first, so a test could reach `Value` before the callback had set it. The fixture now exposes a `Connected` signal the launch helper awaits. 6/6 clean net462 runs and 5/5 clean net8.0 runs afterwards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

+ if (serverTask is not null)
+ {
+ // The launch is being abandoned, so skip the graceful wait entirely: there is no connected
+ // transport whose closure could signal the callback, and the caller (often a canceling one) is
+ // waiting on this unwind. A zero graceful timeout goes straight to cancel-then-grace.
+ if (!await ShutdownServerAsync(serverTask, serverCancellation!, TimeSpan.Zero, logger).ConfigureAwait(false))
+ {
+ // The callback is still running and still holds the token; disposing its source now would
+ // turn a clean abandonment into an ObjectDisposedException inside the caller's own code.
+ throw;
+ }
+ }


github-actions · 2026-08-31T19:32:23Z

🧪 Expert test review — PR #10898

Grade	Test	Mutation	Notes	How to improve
B (80–89)	new `MtpServerClientInProcessTests. Dispose_CallbackFaultsDuringShutdown_ DoesNotThrow`	2/3 killed	Reads `server.Completion.Exception` but never asserts on it, so a mutation changing the faulted exception's identity/message survives.	Assert the captured exception's message/type instead of discarding it after touching `.Exception`.
B (80–89)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_HonorsTheStatefulOption`	3/4 killed	Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on `client.Capabilities` is not caught.	Also assert `client.Capabilities.IsStateful` reflects the negotiated round-trip value, not only the sent request.
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_PassesCompleteServerModeArguments`	5/5 killed	Verifies exact ordered arguments, dynamic port and fixed count; strong regression guard for the generated argument array.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_DrivesInitializeDiscoverRunAndExit`	6/6 killed	End-to-end drive through initialize/discover/run/exit with distinct assertions per state transition.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsBeforeConnecting_ PreservesCallbackException`	2/2 killed	Asserts the exact exception instance is preserved as inner exception, not just its type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsAsynchronouslyBeforeConnecting_ PreservesCallbackException`	2/2 killed	Covers the async-throw variant with the same identity assertion as the sync case.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsWithoutConnecting_ FailsFastInsteadOfWaitingOutTheTimeout`	3/3 killed	Checks both the reported exit code in the message and a timing bound guarding against a slow-timeout regression.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsBeforeConnecting_ ReportsExitCode`	2/2 killed	Asserts the specific exit code surfaces in the exception message.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackReturnsNullTask_Fails`	2/2 killed	Asserts the specific inner exception type for a misbehaving callback returning a null task.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_NullCallback_Throws`	1/1 killed	Focused single-assertion guard-clause test using the exact exception type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_AlreadyCanceled_ DoesNotInvokeCallback`	2/2 killed	Verifies both the cancellation exception and, via an interlocked counter, that the callback never ran.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CanceledWhileConnecting_ CancelsTheCallbackToken`	2/2 killed	Confirms both the caller-side cancellation and that the callback's own token observed cancellation.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_ConnectionTimeoutElapses_ FailsWithTimeoutMessage`	3/3 killed	Distinguishes the connection timeout from the deliberately huge shutdown timeout via message and elapsed-time bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_ClosesTransportAndAwaitsTheCallback`	4/4 killed	Checks pre/post completion state, the callback's returned exit code, and the client's reported exit code.	—
A (90–100)	new `MtpServerClientInProcessTests. ShutdownAsync_ClosesTransportAndAwaitsTheCallback_ WithoutBlocking`	4/4 killed	Verifies await-completion, exit code, a fast follow-up Dispose, and single-teardown count together.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_FromANotificationHandler_ DoesNotSelfWaitOnTheReadLoop`	1/1 killed	Targeted timing assertion guards against a specific re-entrancy deadlock regression.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_WhileShutdownAsyncIsInFlight_ WaitsForTheSameTeardown`	3/3 killed	Uses a controlled release gate to prove the racing Dispose genuinely blocks, then confirms a single shared teardown.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_IsIdempotent`	3/3 killed	Confirms exactly one transport close and one completion despite three Dispose calls, plus a fast-return bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_CallbackIgnoresShutdown_ ReturnsWithinTheDocumentedBound`	2/2 killed	Verifies both the documented abandonment time bound and the logged "abandoning" diagnostic.	—
A (90–100)	new `MtpServerClientInProcessTests. RunTestsAsync_Canceled_ SendsCancelRequestToTheHostedApplication`	2/2 killed	Confirms both the client-side cancellation exception and the wire-level cancel notification reaching the fake server.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_MultipleRequestsOnOneSession_ ReuseTheSameConnection`	3/3 killed	Checks keep-alive negotiation, single-connection reuse count, and total request count together.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_IgnoresEnvironmentVariablesAndWarns`	1/1 killed	Asserts the specific warning content naming the ignored option.	—
A (90–100)	new `MtpServerClientInProcessAcceptanceTests. InProcessHost_ DiscoversAndRunsItsOwnMSTestNodes_ WithoutStartingAProcess`	5/5 killed	Real end-to-end embedded-host run asserting build success, exit code, and each marker line the generated app emits.	—
A (90–100)	mod `MtpServerClientAcceptanceTests. DiscoverAndRun_ViaSourcePackageClient_ ReportsExpectedTestNode`	1/1 killed	New lines assert the external-process ShutdownAsync/Dispose share a teardown and report an exit code.	—
A (90–100)	mod `MtpServerClientSourcePackageConsumerTests. HostileConsumer_ CompilesAgainstPackedSource`	N/A	Compile-only guard; added lines correctly extend surface coverage to `ShutdownAsync` and the in-process launch path.	—

_{This advisory comment was generated automatically. Grades are heuristic

and informational — they do not block merging. Suggestions on the Files

changed tab can be applied with one click. Re-run with
/review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · [◷]( · ◷)

github-actions · 2026-08-31T19:32:24Z

🧵 Parallel-safety audit — PR #10898

Parallelization — audited assemblies:

Test assembly	Scope	Workers	Analyzer coverage
`MSTest.Acceptance.IntegrationTests`	`MethodLevel` (default `[assembly: Parallelize(Scope = ExecutionScope.MethodLevel, Workers = 0)]` in `Program.cs`)	CPU count	coverable once the parallel-safety analyzers ship (attribute-based opt-in)
`Microsoft.Testing.Platform.Acceptance.IntegrationTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped
`Microsoft.Testing.Platform.ServerMode.Client.Sources.UnitTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped

This PR did not touch any .runsettings/testconfig.json/Directory.Build.* parallelization config — no assembly's scope changed.

Findings: A (global-state) 0 · B (paths) 0 · C (declaration) 0 · D (over-serialization) 0 — by severity: Critical 0 · High 0 · Warning 0 · Info 0.

No unsafe call sites found in the lines this PR added or modified.

MtpServerClientAcceptanceTests.cs — added lines only call client.ShutdownAsync() / assert ServerExitCode; no process-global state touched.
MtpServerClientSourcePackageConsumerTests.cs — added lines call into an isolated generated test asset (DriveAsync/DriveInProcessAsync); no shared/relative paths, env vars, culture, or console state mutated at the assembly level (Console.WriteLine here writes to the generated app's own redirected output, not the auditing process).
MtpServerClientInProcessAcceptanceTests.cs (new) — uses TestAsset.GenerateAssetAsync, which allocates a unique GUID-suffixed TempDirectory per instance (TestAsset.cs:19-26, throws if a path already exists) — no shared-path collision. CreateChildEnvironment() builds a fresh Dictionary passed only to the spawned child process's environment, not Environment.SetEnvironmentVariable, so it does not mutate this process's state.
FakeMtpServer.cs / MtpServerClientInProcessTests.cs (new) — each test constructs its own TcpListener/FakeMtpServer bound to an OS-assigned loopback port (new TcpListener(IPAddress.Loopback, 0)), so there is no fixed-port collision between concurrently-running tests; no static/process-global state is written.

Nothing here implies a testability or smell concern beyond this audit's scope.

_{Advisory only — heuristic, non-blocking. Re-run with /parallel-audit. This audit answers "is it parallel-safe?"; for testability, smells, or flakiness see the detect-static-dependencies / test-smell-detection / test-anti-patterns analyses.}

🤖 Automated content by GitHub Copilot. Generated by the Parallel-safety audit on PR (on open / sync) workflow. · auto · 117.1 AIC · ⌖ 3.53 AIC · ⊞ 24.8K · [◷]( · ◷)

github-actions

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · ◷

github-actions · 2026-08-31T19:32:25Z

+ client.Dispose();
+
+ Assert.IsTrue(server.Completion.IsFaulted);
+ _ = server.Completion.Exception;


🧪 Test review · Grade B (80–89) — Reads server.Completion.Exception but never asserts on it, so a mutation changing the faulted exception's identity/message would survive.
Assert the captured exception's message/type instead of discarding it after touching .Exception.
Suggested change
_=server.Completion.Exception;
Exception?faultException=server.Completion.Exception?.GetBaseException();
Assert.IsInstanceOfType<InvalidOperationException>(faultException);
Assert.AreEqual("The application failed while shutting down.",faultException!.Message);

github-actions · 2026-08-31T19:32:25Z

+ ? typed
+ : SerializerUtilities.Deserialize<InitializeRequestArgs>((IDictionary<string, object?>)initialize.Params!);
+
+ Assert.AreEqual(isStateful, args.Capabilities.IsStateful, "The in-process path must forward the client's stateful capability unchanged.");


🧪 Test review · Grade B (80–89) — Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on client.Capabilities is not caught.
Also assert client.Capabilities.IsStateful reflects the negotiated round-trip value, not only the sent request.
Suggested change
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,capabilities.IsStateful,"The negotiated capability the client stores must reflect the server's round trip, not just the sent request.");

github-actions

Note

🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

✅ 22/22 dimensions clean — no findings.

Summary: This PR adds MtpServerClient.LaunchInProcessAsync for embedded hosts (MAUI, Android/iOS) that cannot Process.Start. The implementation is thorough and well-structured:

Threading & Concurrency: Shutdown is idempotent via lock + lazy Task, Volatile.Read/Write for cross-thread exit code, Task.Run avoids sync-context deadlocks. No shared mutable state without synchronization.
Security & IPC: Loopback TCP only, no command-line injection surface (arguments are array-based), WASM fails fast with PlatformNotSupportedException.
Public API: All new API is internal (source-only package). No init accessors. No PublicAPI.Unshipped.txt changes needed.
Performance: GetCurrentProcessId() snapshotted once in constructor. No hot-path allocations. WaitBoundedAsync clamps timeouts correctly.
Cross-TFM: #if NET8_0_OR_GREATER guard on AcceptTcpClientAsync(CancellationToken). RuntimeInformation used for OS detection on net462.
Resource Management: Every disposable (TcpListener, TcpClient, CancellationTokenSource, Process) has cleanup in both success and error paths. Pending accepts are neutralized. ServerCancellation is only disposed when safe.
Defensive Coding: Callback exceptions are wrapped as InnerException in MtpServerConnectionClosedException. Null task from callback is caught. Cancel() runs on thread pool to avoid blocking registration.
Error Handling: Shared teardown task wrapped in catch-all so it never faults. ObserveFailure prevents UnobservedTaskException. Every teardown helper is non-throwing.
Tests: 68 unit tests + acceptance tests covering argument shape, lifecycle, faults, cancellation, timeouts, idempotent disposal, notification-handler disposal, and the real end-to-end in-process path.
Documentation: Changelog, protocol intro, PACKAGE.md all updated. XML doc comments are thorough.

The refactoring of shared transport logic into MtpServerConnector and the IMtpServerHost abstraction is clean and prevents the two launch paths from drifting. The existing LaunchAsync(path) path is provably unchanged (same argument string, same failure messages, same teardown order).

Copilot

Copilot review overview

Review tier: Balanced
Findings: 3

New issues introduced by this change (3)

Severity	Finding
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `MtpJsonRpcConnection.Dispose()` closes the socket and then waits up to its 5-second read-loop…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `ShutdownAsync` writes this nullable property from the teardown worker while callers can read…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — A canceled callback takes this branch without an inner exception, although the package…

What changed in this PR

Adds in-process launch support to the source-only MTP server-mode client for embedded/mobile hosts, addressing #10890 and enabling scenarios related to #9809.

Changes:

Adds callback-based in-process hosting with shared transport setup.
Adds bounded asynchronous shutdown and server exit-code reporting.
Adds documentation, unit coverage, and real-MSTest acceptance coverage.

File	Description
`MtpServerClientInProcessTests.cs`	Tests in-process lifecycle and protocol behavior.
`FakeMtpServer.cs`	Supports server-to-client dial-back mode.
`MtpServerClientInProcessAcceptanceTests.cs`	Exercises a real in-process MSTest application.
`MtpServerClientAcceptanceTests.cs`	Covers external-process shutdown and exit code.
`MtpServerClientSourcePackageConsumerTests.cs`	Extends source-package compile coverage.
`PACKAGE.md`	Documents embedded-host usage and lifecycle.
`MtpServerProcess.cs`	Adopts shared host and shutdown abstractions.
`MtpServerInProcessHost.cs`	Implements callback hosting and teardown.
`MtpServerConnector.cs`	Centralizes listener and transport setup.
`MtpServerClientOptions.cs`	Adds the shutdown timeout option.
`MtpServerClientExceptions.cs`	Supports preserved inner exceptions.
`MtpServerClient.cs`	Exposes in-process launch and shutdown.
`IMtpServerHost.cs`	Defines common host ownership behavior.
`IMtpServerClient.cs`	Adds shutdown and exit-code members.
`001-protocol-intro.md`	Documents reference-client launch modes.
`Changelog-Platform.md`	Records the new embedded-host capability.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+ Connection.Dispose();
+ SafeDispose(_client, _logger, "Disposing the accepted client socket");
+ MtpServerConnector.SafeStop(_listener, _logger);
+
+ bool stopped = await ShutdownServerAsync(_serverTask, _serverCancellation, _shutdownTimeout, _logger).ConfigureAwait(false);


+ /// Gets the exit code the hosted application returned, or <see langword="null"/> while it is still
+ /// running (or when it failed or was abandoned rather than returning one).
+ /// </summary>
+ public int? ExitCode { get; private set; }


+ if (serverTask.IsCanceled)
+ {
+ return new MtpServerConnectionClosedException(
+ "The in-process Microsoft.Testing.Platform application was canceled before connecting back.");
+ }


Amaury Levé (Evangelink) · 2026-08-31T19:22:25Z

Fixes#10890

Why

MtpServerClient.LaunchAsync(path) owns the loopback listener and starts the test application with Process.Start. That is right for IDE and desktop tooling, but unusable for embedded hosts — MAUI apps, Android/iOS test apps — where the MTP application already runs in the caller's process.

Today those hosts have to reimplement the listener, the --server/host/port arguments, the race between connection and startup failure, the serializer-before-formatter registration order, TcpMessageHandler + MtpJsonRpcConnection construction, and the exit / transport-close / server-completion shutdown dance. That defeats the point of shipping a canonical client and makes it easy to reintroduce bugs this package already solved: partial frame writes, ignored JSON-RPC errors, missing $/cancelRequest, unbounded waits, exception masking.

A concrete consumer is the DeviceRunners MSTest visual-runner work (mattleibow/DeviceRunners#157, #9809).

What

usingIMtpServerClientclient=awaitMtpServerClient.LaunchInProcessAsync(async(serverArgs,token)=>{ITestApplicationBuilderbuilder=awaitTestApplication.CreateBuilderAsync(serverArgs);builder.AddMSTest(()=>testAssemblies);usingITestApplicationapp=awaitbuilder.BuildAsync();returnawaitapp.RunAsync();},options,cancellationToken);awaitclient.InitializeAsync();awaitclient.DiscoverTestsAsync();awaitclient.RunTestsAsync();awaitclient.ExitAsync();awaitclient.ShutdownAsync();

The caller supplies only "how to run the application". The client keeps everything else: it binds the listener, generates the complete argument array (--server jsonrpc --client-host 127.0.0.1 --client-port <port> --no-banner), races the connect, builds the transport, and owns a bounded shutdown.

New API on the injected (still internal) surface:

Member	Purpose
`MtpServerClient.LaunchInProcessAsync(callback, options, ct)`	The embedded-host launch path. Async only — blocking the launching thread can deadlock the application being launched.
`IMtpServerClient.ShutdownAsync()`	Non-blocking teardown.
`IMtpServerClient.ServerExitCode`	The value the application returned.
`MtpServerClientOptions.ServerShutdownTimeout`	Graceful shutdown bound (default 30s).

Internally, the transport setup was extracted from MtpServerProcess into MtpServerConnector, and both launch paths now sit behind IMtpServerHost, so they cannot drift.

Behavior worth reviewing

Ownership and shutdown. Both Dispose() and ShutdownAsync() join one lazily created shared teardown task, on both hosts. A Dispose that follows or races ShutdownAsync therefore returns only once the server has actually stopped, rather than reporting success while teardown is still running. Teardown runs on the thread pool, so ShutdownAsync never blocks and the synchronous Dispose cannot deadlock against a UI-thread continuation. Task.Run captures the execution context, so the connection's read-loop AsyncLocal marker still flows and disposing from a notification handler does not self-wait (covered by a test asserting < 4s against the connection's 5s read-loop timeout).

Bounded, and actually bounded. Teardown waits ServerShutdownTimeout, then cancels the callback's token, then a fixed 5s grace, then abandons and logs. CancellationTokenSource.Cancel() runs registrations synchronously, so the cancellation is started separately — otherwise a blocking caller registration would prevent the grace from ever starting and make the "bounded" wait unbounded. A failed launch skips the graceful wait entirely: nothing is connected, so there is no transport closure for the callback to observe.

Exception preservation. A callback that throws, is canceled, or returns before dialing back surfaces as MtpServerConnectionClosedException with the original as InnerException, instead of a misleading connection timeout. Every teardown helper is non-throwing, and the shared teardown task is wrapped so it can never fault — a faulted shared task would throw from every later disposal.

LaunchAsync(path) is unchanged. Verified against main: identical argument string, failure messages, stderr capture, exit fast-fail and teardown order. One earlier revision of this branch added a grace period to the accept race; review showed it only traded a precise failure for a vague one, so it was removed and the external path is now provably unchanged.

Browser/WASM. Both paths are loopback TCP. LaunchInProcessAsync fails fast with PlatformNotSupportedException there; this does not enable WASM hosting, and the docs say so.

Tests

68 unit tests (net8.0) / 63 (net462, Jsonite path). New coverage: argument array shape; initialize → discover → run → exit; callback faulting synchronously and asynchronously; callback exiting with a code; null task; pre-canceled launch not invoking the callback; cancellation during connect canceling the callback token; connection timeout bounded by the grace rather than ServerShutdownTimeout; ShutdownAsync; ServerExitCode; disposal awaiting the callback; Dispose racing an in-flight ShutdownAsync; Dispose from a notification handler; idempotent disposal; callback faulting during shutdown; unresponsive callback abandoned within the bound; $/cancelRequest; stateful on/off; multi-request single connection; EnvironmentVariables ignored and warned. Stressed 6×/5× consecutively for flakiness.
New acceptance test where a single generated process is simultaneously the embedded host (compiling the packed source-only package) and a real MTP TestApplication with real MSTest over a real [TestClass] — discovering and running its own test over JSON-RPC with no Process.Start anywhere. It also asserts the server's reported process id equals its own.
Existing external-process acceptance test extended to cover ShutdownAsync + ServerExitCode; hostile-consumer compile oracle extended to bind the new API on net462/netstandard2.0/net5.0–net8.0.
Full regression: 13 platform source-package/consumer acceptance, 4 MSTest acceptance, 13 platform ServerTests. build.cmd -pack clean, 0 warnings.

Review history

Four independent review rounds (MTP/MSTest expert reviewer, a design reviewer, and two correctness reviewers) produced 3 major, 10 moderate and 10 minor findings, all addressed. Several were real bugs the tests then locked in — notably Process.ExitCode being unreadable after Process.Dispose(), a TcpListener socket leak when Start() failed, and a teardown path that could throw from a contract documented never to throw.

Open design questions

No IAsyncDisposable. netstandard2.0/net462 would need Microsoft.Bcl.AsyncInterfaces, breaking the package's dependency-free promise. ShutdownAsync() is the substitute. Happy to revisit if the dependency is acceptable.
MtpServerClientOptions is mode-mixed.EnvironmentVariables is external-process only; ServerShutdownTimeout is in-process only. Nesting per-transport options would age better, but EnvironmentVariables already shipped at the top level, so it cannot be done non-breakingly now.
The 5s cancellation grace is a fixed constant, not an option.
The two paths still build different argument shapes — deliberate, to keep the shipped external command line byte-identical. Commented at the call site.
No ConnectAsync(TcpClient/Stream, options) factory — the existing MtpServerClient(MtpJsonRpcConnection, options) constructor already covers "wrap an existing transport", and the guidance is to minimize injected surface. Easy to add if wanted.

Embedded hosts such as MAUI or Android/iOS test apps cannot spawn a child process, so `MtpServerClient.LaunchAsync(path)` was unusable for them and they had to reimplement the listener, the server-mode arguments, the connect race, the serializer/formatter ordering, the transport and the shutdown coordination by hand -- reintroducing the bugs the canonical client already solves. Add `MtpServerClient.LaunchInProcessAsync(callback, options, cancellationToken)`. The client keeps ownership of everything except "how to run the application": it binds the loopback listener, generates the complete server-mode argument array, races the connect against callback failure/completion, caller cancellation and the connection timeout, and owns a bounded shutdown that closes the transport, then cancels the callback token, then abandons it rather than hanging the caller. A callback failure before connection is surfaced as `MtpServerConnectionClosedException` with the caller's exception preserved as the inner exception, and teardown failures are only logged so they can never replace the primary failure. The shared transport setup is extracted from `MtpServerProcess` into `MtpServerConnector` and both launch paths now flow through `IMtpServerHost`, so the external-process behavior is unchanged while the two paths cannot drift. The one behavior change is an improvement shared by both: when the server is seen to have stopped, a still-pending accept gets a bounded final grace so a connection established just before the stop is not discarded in favor of a misleading "stopped before connecting back" failure. The path is loopback TCP, so it fails fast with `PlatformNotSupportedException` on browser/WASM; it does not enable WASM hosting. Fixes#10890 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Two independent reviews of the in-process launch path raised the same top finding: `Dispose()` waits for the hosted application synchronously on the calling thread, and on the very platforms this feature targets (MAUI, Android, iOS) a multi-second block on the UI thread trips the ANR / watchdog. Add `IMtpServerClient.ShutdownAsync()` (and `IMtpServerHost.ShutdownAsync`) so the same teardown can be awaited instead. `IAsyncDisposable` stays rejected -- netstandard2.0 and net462 would need `Microsoft.Bcl.AsyncInterfaces`, which breaks the package's dependency-free promise -- but rejecting the interface is not a reason to have no asynchronous shutdown at all. `Dispose()` remains, is still idempotent, and returns immediately after `ShutdownAsync`. Remove the accept grace period. The previous commit let a still-pending accept win for ~100ms after the server was seen to have stopped, on the theory that a connection established just before the stop should not be discarded. That theory does not hold: if the server has stopped, the socket belongs to a dead peer, so the grace only traded a precise failure (exit code plus captured stderr) for a generic connection-closed error on the first request, and its uncancellable delay could let the server failure beat a concurrent caller cancellation. Dropping it also makes the external-process path provably unchanged, so the changelog no longer needs a `Changed` entry. Other review fixes: * Expose `IMtpServerClient.ServerExitCode`. The callback signature already demanded a `Task<int>`, but after a successful session the value was unreachable, so an embedded host whose `Main` must return it had to capture it in a closure. * Do not dispose the server's `CancellationTokenSource` when the callback was abandoned while still running: it holds the token, and `token.WaitHandle` or `CreateLinkedTokenSource` would then throw `ObjectDisposedException` inside the caller's own code. * Skip the graceful wait entirely on a failed launch. Nothing is connected, so there is no transport closure for the callback to observe; only the fixed cancellation grace applies and an unwinding caller no longer pays `ServerShutdownTimeout`. * Clamp bounded waits instead of trusting the caller's `TimeSpan`. A negative value (or `Timeout.InfiniteTimeSpan`) made `Task.Delay` throw from a path documented never to throw; an oversized one is capped to the largest delay .NET Framework accepts. * Move `TcpListener.Start()` inside the cleanup `try`: it creates the socket before binding, so a bind failure leaked it because the caller never received a listener it could stop. * Log a late failure from an abandoned callback rather than only observing it. * Cache the in-process host's `ProcessId` instead of allocating a `Process` per property read. * Document that `Dispose()` blocks, that cancellation is bounded rather than immediate, and why the two launch paths still build different argument shapes; fix the shadowed `cancellationToken` in the PACKAGE.md sample. Tests: cover `ShutdownAsync`, `ServerExitCode`, and fail-fast on a server that stops without connecting; make `Dispose_IsIdempotent` able to fail by asserting on observed transport closes rather than callback invocations; replace the hand-rolled throws helper with `Assert.ThrowsExactlyAsync`; tighten the shutdown-bound assertion; and stop leaking `Process` handles. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

A third review round found three ways the teardown contract did not hold up under concurrency. `Dispose()` and `ShutdownAsync()` each had their own `Interlocked.Exchange` guard, so whichever call lost the race returned while teardown was still running -- reporting to its caller that the application had stopped when it had not. Replace both guards with one lazily started, shared teardown task: every entry point now joins the same task, so `Dispose` after (or racing) `ShutdownAsync` returns only once the application has actually stopped, and the whole thing stays idempotent. `MtpServerClient` no longer keeps its own guard either; it delegates to the host, whose teardown is the single source of truth. `ShutdownAsync()` could still block the very thread it exists to protect: it closed the transport before its first await, and closing the connection waits up to five seconds for the read loop. The shared task is started with `Task.Run`, so the whole teardown -- transport close included -- runs on the thread pool and `ShutdownAsync` returns immediately. `MtpServerProcess` likewise now runs its (bounded but synchronous) kill off the calling thread rather than pretending to be async while blocking. `CancellationTokenSource.Cancel()` executes registrations synchronously on the calling thread, so a caller registration that blocked would prevent the five-second cancellation grace from ever starting and make the "bounded" shutdown unbounded. Start the cancellation separately and begin the grace regardless. When a registration is still executing once the callback has finished, the source is reported as unsafe to dispose for the same reason an abandoned callback is: leaking one `CancellationTokenSource` beats a use-after-dispose inside the caller's code. Adds a test that a `Dispose` racing an in-flight `ShutdownAsync` blocks until the shared teardown completes, and that both share one teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

… fix ExitCode A fourth review round found that only the in-process host had been converted to the shared-teardown design. `MtpServerProcess` kept its old early-return guard, so on the external-process path a `Dispose()` that followed or raced `ShutdownAsync()` returned while `SafeKill`'s bounded `WaitForExit` was still running on another thread -- losing exactly the guarantee that wait exists for (a caller may delete the application directory immediately after disposal), and making `await ShutdownAsync()` report that the server had stopped when it had not. That contradicted the comment the previous commit added to `MtpServerClient.Dispose`, which claimed both paths joined an in-flight teardown. Convert `MtpServerProcess` to the same lazily created shared task, so the contract is now uniform across both implementations of `IMtpServerHost`, and reconcile the interface docs, which previously described the opposite rule. Adding real coverage for `ShutdownAsync` on the external-process path then surfaced a genuine bug: `MtpServerProcess.ExitCode` was always `null` after teardown, because a `Process` cannot be queried once disposed. Capture the exit code during teardown instead -- before the kill, so an application that already exited on its own reports its real code rather than the kill's, and before `Process.Dispose()`, after which nothing is readable. Also fixes a race in the in-process test fixture that made the suite flaky on net462: the callback published its `FakeMtpServer` only after its own connect call returned, but the client's accept can complete first, so a test could reach `Value` before the callback had set it. The fixture now exposes a `Connected` signal the launch helper awaits. 6/6 clean net462 runs and 5/5 clean net8.0 runs afterwards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

+ if (serverTask is not null)
+ {
+ // The launch is being abandoned, so skip the graceful wait entirely: there is no connected
+ // transport whose closure could signal the callback, and the caller (often a canceling one) is
+ // waiting on this unwind. A zero graceful timeout goes straight to cancel-then-grace.
+ if (!await ShutdownServerAsync(serverTask, serverCancellation!, TimeSpan.Zero, logger).ConfigureAwait(false))
+ {
+ // The callback is still running and still holds the token; disposing its source now would
+ // turn a clean abandonment into an ObjectDisposedException inside the caller's own code.
+ throw;
+ }
+ }


github-actions · 2026-08-31T19:32:23Z

🧪 Expert test review — PR #10898

Grade	Test	Mutation	Notes	How to improve
B (80–89)	new `MtpServerClientInProcessTests. Dispose_CallbackFaultsDuringShutdown_ DoesNotThrow`	2/3 killed	Reads `server.Completion.Exception` but never asserts on it, so a mutation changing the faulted exception's identity/message survives.	Assert the captured exception's message/type instead of discarding it after touching `.Exception`.
B (80–89)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_HonorsTheStatefulOption`	3/4 killed	Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on `client.Capabilities` is not caught.	Also assert `client.Capabilities.IsStateful` reflects the negotiated round-trip value, not only the sent request.
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_PassesCompleteServerModeArguments`	5/5 killed	Verifies exact ordered arguments, dynamic port and fixed count; strong regression guard for the generated argument array.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_DrivesInitializeDiscoverRunAndExit`	6/6 killed	End-to-end drive through initialize/discover/run/exit with distinct assertions per state transition.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsBeforeConnecting_ PreservesCallbackException`	2/2 killed	Asserts the exact exception instance is preserved as inner exception, not just its type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsAsynchronouslyBeforeConnecting_ PreservesCallbackException`	2/2 killed	Covers the async-throw variant with the same identity assertion as the sync case.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsWithoutConnecting_ FailsFastInsteadOfWaitingOutTheTimeout`	3/3 killed	Checks both the reported exit code in the message and a timing bound guarding against a slow-timeout regression.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsBeforeConnecting_ ReportsExitCode`	2/2 killed	Asserts the specific exit code surfaces in the exception message.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackReturnsNullTask_Fails`	2/2 killed	Asserts the specific inner exception type for a misbehaving callback returning a null task.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_NullCallback_Throws`	1/1 killed	Focused single-assertion guard-clause test using the exact exception type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_AlreadyCanceled_ DoesNotInvokeCallback`	2/2 killed	Verifies both the cancellation exception and, via an interlocked counter, that the callback never ran.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CanceledWhileConnecting_ CancelsTheCallbackToken`	2/2 killed	Confirms both the caller-side cancellation and that the callback's own token observed cancellation.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_ConnectionTimeoutElapses_ FailsWithTimeoutMessage`	3/3 killed	Distinguishes the connection timeout from the deliberately huge shutdown timeout via message and elapsed-time bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_ClosesTransportAndAwaitsTheCallback`	4/4 killed	Checks pre/post completion state, the callback's returned exit code, and the client's reported exit code.	—
A (90–100)	new `MtpServerClientInProcessTests. ShutdownAsync_ClosesTransportAndAwaitsTheCallback_ WithoutBlocking`	4/4 killed	Verifies await-completion, exit code, a fast follow-up Dispose, and single-teardown count together.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_FromANotificationHandler_ DoesNotSelfWaitOnTheReadLoop`	1/1 killed	Targeted timing assertion guards against a specific re-entrancy deadlock regression.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_WhileShutdownAsyncIsInFlight_ WaitsForTheSameTeardown`	3/3 killed	Uses a controlled release gate to prove the racing Dispose genuinely blocks, then confirms a single shared teardown.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_IsIdempotent`	3/3 killed	Confirms exactly one transport close and one completion despite three Dispose calls, plus a fast-return bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_CallbackIgnoresShutdown_ ReturnsWithinTheDocumentedBound`	2/2 killed	Verifies both the documented abandonment time bound and the logged "abandoning" diagnostic.	—
A (90–100)	new `MtpServerClientInProcessTests. RunTestsAsync_Canceled_ SendsCancelRequestToTheHostedApplication`	2/2 killed	Confirms both the client-side cancellation exception and the wire-level cancel notification reaching the fake server.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_MultipleRequestsOnOneSession_ ReuseTheSameConnection`	3/3 killed	Checks keep-alive negotiation, single-connection reuse count, and total request count together.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_IgnoresEnvironmentVariablesAndWarns`	1/1 killed	Asserts the specific warning content naming the ignored option.	—
A (90–100)	new `MtpServerClientInProcessAcceptanceTests. InProcessHost_ DiscoversAndRunsItsOwnMSTestNodes_ WithoutStartingAProcess`	5/5 killed	Real end-to-end embedded-host run asserting build success, exit code, and each marker line the generated app emits.	—
A (90–100)	mod `MtpServerClientAcceptanceTests. DiscoverAndRun_ViaSourcePackageClient_ ReportsExpectedTestNode`	1/1 killed	New lines assert the external-process ShutdownAsync/Dispose share a teardown and report an exit code.	—
A (90–100)	mod `MtpServerClientSourcePackageConsumerTests. HostileConsumer_ CompilesAgainstPackedSource`	N/A	Compile-only guard; added lines correctly extend surface coverage to `ShutdownAsync` and the in-process launch path.	—

_{This advisory comment was generated automatically. Grades are heuristic

and informational — they do not block merging. Suggestions on the Files

changed tab can be applied with one click. Re-run with
/review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · [◷]( · ◷)

github-actions · 2026-08-31T19:32:24Z

🧵 Parallel-safety audit — PR #10898

Parallelization — audited assemblies:

Test assembly	Scope	Workers	Analyzer coverage
`MSTest.Acceptance.IntegrationTests`	`MethodLevel` (default `[assembly: Parallelize(Scope = ExecutionScope.MethodLevel, Workers = 0)]` in `Program.cs`)	CPU count	coverable once the parallel-safety analyzers ship (attribute-based opt-in)
`Microsoft.Testing.Platform.Acceptance.IntegrationTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped
`Microsoft.Testing.Platform.ServerMode.Client.Sources.UnitTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped

This PR did not touch any .runsettings/testconfig.json/Directory.Build.* parallelization config — no assembly's scope changed.

Findings: A (global-state) 0 · B (paths) 0 · C (declaration) 0 · D (over-serialization) 0 — by severity: Critical 0 · High 0 · Warning 0 · Info 0.

No unsafe call sites found in the lines this PR added or modified.

MtpServerClientAcceptanceTests.cs — added lines only call client.ShutdownAsync() / assert ServerExitCode; no process-global state touched.
MtpServerClientSourcePackageConsumerTests.cs — added lines call into an isolated generated test asset (DriveAsync/DriveInProcessAsync); no shared/relative paths, env vars, culture, or console state mutated at the assembly level (Console.WriteLine here writes to the generated app's own redirected output, not the auditing process).
MtpServerClientInProcessAcceptanceTests.cs (new) — uses TestAsset.GenerateAssetAsync, which allocates a unique GUID-suffixed TempDirectory per instance (TestAsset.cs:19-26, throws if a path already exists) — no shared-path collision. CreateChildEnvironment() builds a fresh Dictionary passed only to the spawned child process's environment, not Environment.SetEnvironmentVariable, so it does not mutate this process's state.
FakeMtpServer.cs / MtpServerClientInProcessTests.cs (new) — each test constructs its own TcpListener/FakeMtpServer bound to an OS-assigned loopback port (new TcpListener(IPAddress.Loopback, 0)), so there is no fixed-port collision between concurrently-running tests; no static/process-global state is written.

Nothing here implies a testability or smell concern beyond this audit's scope.

_{Advisory only — heuristic, non-blocking. Re-run with /parallel-audit. This audit answers "is it parallel-safe?"; for testability, smells, or flakiness see the detect-static-dependencies / test-smell-detection / test-anti-patterns analyses.}

🤖 Automated content by GitHub Copilot. Generated by the Parallel-safety audit on PR (on open / sync) workflow. · auto · 117.1 AIC · ⌖ 3.53 AIC · ⊞ 24.8K · [◷]( · ◷)

github-actions

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · ◷

github-actions · 2026-08-31T19:32:25Z

+ client.Dispose();
+
+ Assert.IsTrue(server.Completion.IsFaulted);
+ _ = server.Completion.Exception;


🧪 Test review · Grade B (80–89) — Reads server.Completion.Exception but never asserts on it, so a mutation changing the faulted exception's identity/message would survive.
Assert the captured exception's message/type instead of discarding it after touching .Exception.
Suggested change
_=server.Completion.Exception;
Exception?faultException=server.Completion.Exception?.GetBaseException();
Assert.IsInstanceOfType<InvalidOperationException>(faultException);
Assert.AreEqual("The application failed while shutting down.",faultException!.Message);

github-actions · 2026-08-31T19:32:25Z

+ ? typed
+ : SerializerUtilities.Deserialize<InitializeRequestArgs>((IDictionary<string, object?>)initialize.Params!);
+
+ Assert.AreEqual(isStateful, args.Capabilities.IsStateful, "The in-process path must forward the client's stateful capability unchanged.");


🧪 Test review · Grade B (80–89) — Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on client.Capabilities is not caught.
Also assert client.Capabilities.IsStateful reflects the negotiated round-trip value, not only the sent request.
Suggested change
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,capabilities.IsStateful,"The negotiated capability the client stores must reflect the server's round trip, not just the sent request.");

github-actions

Note

🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

✅ 22/22 dimensions clean — no findings.

Summary: This PR adds MtpServerClient.LaunchInProcessAsync for embedded hosts (MAUI, Android/iOS) that cannot Process.Start. The implementation is thorough and well-structured:

Threading & Concurrency: Shutdown is idempotent via lock + lazy Task, Volatile.Read/Write for cross-thread exit code, Task.Run avoids sync-context deadlocks. No shared mutable state without synchronization.
Security & IPC: Loopback TCP only, no command-line injection surface (arguments are array-based), WASM fails fast with PlatformNotSupportedException.
Public API: All new API is internal (source-only package). No init accessors. No PublicAPI.Unshipped.txt changes needed.
Performance: GetCurrentProcessId() snapshotted once in constructor. No hot-path allocations. WaitBoundedAsync clamps timeouts correctly.
Cross-TFM: #if NET8_0_OR_GREATER guard on AcceptTcpClientAsync(CancellationToken). RuntimeInformation used for OS detection on net462.
Resource Management: Every disposable (TcpListener, TcpClient, CancellationTokenSource, Process) has cleanup in both success and error paths. Pending accepts are neutralized. ServerCancellation is only disposed when safe.
Defensive Coding: Callback exceptions are wrapped as InnerException in MtpServerConnectionClosedException. Null task from callback is caught. Cancel() runs on thread pool to avoid blocking registration.
Error Handling: Shared teardown task wrapped in catch-all so it never faults. ObserveFailure prevents UnobservedTaskException. Every teardown helper is non-throwing.
Tests: 68 unit tests + acceptance tests covering argument shape, lifecycle, faults, cancellation, timeouts, idempotent disposal, notification-handler disposal, and the real end-to-end in-process path.
Documentation: Changelog, protocol intro, PACKAGE.md all updated. XML doc comments are thorough.

The refactoring of shared transport logic into MtpServerConnector and the IMtpServerHost abstraction is clean and prevents the two launch paths from drifting. The existing LaunchAsync(path) path is provably unchanged (same argument string, same failure messages, same teardown order).

Copilot

Copilot review overview

Review tier: Balanced
Findings: 3

New issues introduced by this change (3)

Severity	Finding
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `MtpJsonRpcConnection.Dispose()` closes the socket and then waits up to its 5-second read-loop…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `ShutdownAsync` writes this nullable property from the teardown worker while callers can read…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — A canceled callback takes this branch without an inner exception, although the package…

What changed in this PR

Adds in-process launch support to the source-only MTP server-mode client for embedded/mobile hosts, addressing #10890 and enabling scenarios related to #9809.

Changes:

Adds callback-based in-process hosting with shared transport setup.
Adds bounded asynchronous shutdown and server exit-code reporting.
Adds documentation, unit coverage, and real-MSTest acceptance coverage.

File	Description
`MtpServerClientInProcessTests.cs`	Tests in-process lifecycle and protocol behavior.
`FakeMtpServer.cs`	Supports server-to-client dial-back mode.
`MtpServerClientInProcessAcceptanceTests.cs`	Exercises a real in-process MSTest application.
`MtpServerClientAcceptanceTests.cs`	Covers external-process shutdown and exit code.
`MtpServerClientSourcePackageConsumerTests.cs`	Extends source-package compile coverage.
`PACKAGE.md`	Documents embedded-host usage and lifecycle.
`MtpServerProcess.cs`	Adopts shared host and shutdown abstractions.
`MtpServerInProcessHost.cs`	Implements callback hosting and teardown.
`MtpServerConnector.cs`	Centralizes listener and transport setup.
`MtpServerClientOptions.cs`	Adds the shutdown timeout option.
`MtpServerClientExceptions.cs`	Supports preserved inner exceptions.
`MtpServerClient.cs`	Exposes in-process launch and shutdown.
`IMtpServerHost.cs`	Defines common host ownership behavior.
`IMtpServerClient.cs`	Adds shutdown and exit-code members.
`001-protocol-intro.md`	Documents reference-client launch modes.
`Changelog-Platform.md`	Records the new embedded-host capability.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+ Connection.Dispose();
+ SafeDispose(_client, _logger, "Disposing the accepted client socket");
+ MtpServerConnector.SafeStop(_listener, _logger);
+
+ bool stopped = await ShutdownServerAsync(_serverTask, _serverCancellation, _shutdownTimeout, _logger).ConfigureAwait(false);


+ /// Gets the exit code the hosted application returned, or <see langword="null"/> while it is still
+ /// running (or when it failed or was abandoned rather than returning one).
+ /// </summary>
+ public int? ExitCode { get; private set; }


+ if (serverTask.IsCanceled)
+ {
+ return new MtpServerConnectionClosedException(
+ "The in-process Microsoft.Testing.Platform application was canceled before connecting back.");
+ }


Amaury Levé (Evangelink) · 2026-08-31T19:22:25Z

Fixes#10890

Why

MtpServerClient.LaunchAsync(path) owns the loopback listener and starts the test application with Process.Start. That is right for IDE and desktop tooling, but unusable for embedded hosts — MAUI apps, Android/iOS test apps — where the MTP application already runs in the caller's process.

Today those hosts have to reimplement the listener, the --server/host/port arguments, the race between connection and startup failure, the serializer-before-formatter registration order, TcpMessageHandler + MtpJsonRpcConnection construction, and the exit / transport-close / server-completion shutdown dance. That defeats the point of shipping a canonical client and makes it easy to reintroduce bugs this package already solved: partial frame writes, ignored JSON-RPC errors, missing $/cancelRequest, unbounded waits, exception masking.

A concrete consumer is the DeviceRunners MSTest visual-runner work (mattleibow/DeviceRunners#157, #9809).

What

usingIMtpServerClientclient=awaitMtpServerClient.LaunchInProcessAsync(async(serverArgs,token)=>{ITestApplicationBuilderbuilder=awaitTestApplication.CreateBuilderAsync(serverArgs);builder.AddMSTest(()=>testAssemblies);usingITestApplicationapp=awaitbuilder.BuildAsync();returnawaitapp.RunAsync();},options,cancellationToken);awaitclient.InitializeAsync();awaitclient.DiscoverTestsAsync();awaitclient.RunTestsAsync();awaitclient.ExitAsync();awaitclient.ShutdownAsync();

The caller supplies only "how to run the application". The client keeps everything else: it binds the listener, generates the complete argument array (--server jsonrpc --client-host 127.0.0.1 --client-port <port> --no-banner), races the connect, builds the transport, and owns a bounded shutdown.

New API on the injected (still internal) surface:

Member	Purpose
`MtpServerClient.LaunchInProcessAsync(callback, options, ct)`	The embedded-host launch path. Async only — blocking the launching thread can deadlock the application being launched.
`IMtpServerClient.ShutdownAsync()`	Non-blocking teardown.
`IMtpServerClient.ServerExitCode`	The value the application returned.
`MtpServerClientOptions.ServerShutdownTimeout`	Graceful shutdown bound (default 30s).

Internally, the transport setup was extracted from MtpServerProcess into MtpServerConnector, and both launch paths now sit behind IMtpServerHost, so they cannot drift.

Behavior worth reviewing

Ownership and shutdown. Both Dispose() and ShutdownAsync() join one lazily created shared teardown task, on both hosts. A Dispose that follows or races ShutdownAsync therefore returns only once the server has actually stopped, rather than reporting success while teardown is still running. Teardown runs on the thread pool, so ShutdownAsync never blocks and the synchronous Dispose cannot deadlock against a UI-thread continuation. Task.Run captures the execution context, so the connection's read-loop AsyncLocal marker still flows and disposing from a notification handler does not self-wait (covered by a test asserting < 4s against the connection's 5s read-loop timeout).

Bounded, and actually bounded. Teardown waits ServerShutdownTimeout, then cancels the callback's token, then a fixed 5s grace, then abandons and logs. CancellationTokenSource.Cancel() runs registrations synchronously, so the cancellation is started separately — otherwise a blocking caller registration would prevent the grace from ever starting and make the "bounded" wait unbounded. A failed launch skips the graceful wait entirely: nothing is connected, so there is no transport closure for the callback to observe.

Exception preservation. A callback that throws, is canceled, or returns before dialing back surfaces as MtpServerConnectionClosedException with the original as InnerException, instead of a misleading connection timeout. Every teardown helper is non-throwing, and the shared teardown task is wrapped so it can never fault — a faulted shared task would throw from every later disposal.

LaunchAsync(path) is unchanged. Verified against main: identical argument string, failure messages, stderr capture, exit fast-fail and teardown order. One earlier revision of this branch added a grace period to the accept race; review showed it only traded a precise failure for a vague one, so it was removed and the external path is now provably unchanged.

Browser/WASM. Both paths are loopback TCP. LaunchInProcessAsync fails fast with PlatformNotSupportedException there; this does not enable WASM hosting, and the docs say so.

Tests

68 unit tests (net8.0) / 63 (net462, Jsonite path). New coverage: argument array shape; initialize → discover → run → exit; callback faulting synchronously and asynchronously; callback exiting with a code; null task; pre-canceled launch not invoking the callback; cancellation during connect canceling the callback token; connection timeout bounded by the grace rather than ServerShutdownTimeout; ShutdownAsync; ServerExitCode; disposal awaiting the callback; Dispose racing an in-flight ShutdownAsync; Dispose from a notification handler; idempotent disposal; callback faulting during shutdown; unresponsive callback abandoned within the bound; $/cancelRequest; stateful on/off; multi-request single connection; EnvironmentVariables ignored and warned. Stressed 6×/5× consecutively for flakiness.
New acceptance test where a single generated process is simultaneously the embedded host (compiling the packed source-only package) and a real MTP TestApplication with real MSTest over a real [TestClass] — discovering and running its own test over JSON-RPC with no Process.Start anywhere. It also asserts the server's reported process id equals its own.
Existing external-process acceptance test extended to cover ShutdownAsync + ServerExitCode; hostile-consumer compile oracle extended to bind the new API on net462/netstandard2.0/net5.0–net8.0.
Full regression: 13 platform source-package/consumer acceptance, 4 MSTest acceptance, 13 platform ServerTests. build.cmd -pack clean, 0 warnings.

Review history

Four independent review rounds (MTP/MSTest expert reviewer, a design reviewer, and two correctness reviewers) produced 3 major, 10 moderate and 10 minor findings, all addressed. Several were real bugs the tests then locked in — notably Process.ExitCode being unreadable after Process.Dispose(), a TcpListener socket leak when Start() failed, and a teardown path that could throw from a contract documented never to throw.

Open design questions

No IAsyncDisposable. netstandard2.0/net462 would need Microsoft.Bcl.AsyncInterfaces, breaking the package's dependency-free promise. ShutdownAsync() is the substitute. Happy to revisit if the dependency is acceptable.
MtpServerClientOptions is mode-mixed.EnvironmentVariables is external-process only; ServerShutdownTimeout is in-process only. Nesting per-transport options would age better, but EnvironmentVariables already shipped at the top level, so it cannot be done non-breakingly now.
The 5s cancellation grace is a fixed constant, not an option.
The two paths still build different argument shapes — deliberate, to keep the shipped external command line byte-identical. Commented at the call site.
No ConnectAsync(TcpClient/Stream, options) factory — the existing MtpServerClient(MtpJsonRpcConnection, options) constructor already covers "wrap an existing transport", and the guidance is to minimize injected surface. Easy to add if wanted.

Embedded hosts such as MAUI or Android/iOS test apps cannot spawn a child process, so `MtpServerClient.LaunchAsync(path)` was unusable for them and they had to reimplement the listener, the server-mode arguments, the connect race, the serializer/formatter ordering, the transport and the shutdown coordination by hand -- reintroducing the bugs the canonical client already solves. Add `MtpServerClient.LaunchInProcessAsync(callback, options, cancellationToken)`. The client keeps ownership of everything except "how to run the application": it binds the loopback listener, generates the complete server-mode argument array, races the connect against callback failure/completion, caller cancellation and the connection timeout, and owns a bounded shutdown that closes the transport, then cancels the callback token, then abandons it rather than hanging the caller. A callback failure before connection is surfaced as `MtpServerConnectionClosedException` with the caller's exception preserved as the inner exception, and teardown failures are only logged so they can never replace the primary failure. The shared transport setup is extracted from `MtpServerProcess` into `MtpServerConnector` and both launch paths now flow through `IMtpServerHost`, so the external-process behavior is unchanged while the two paths cannot drift. The one behavior change is an improvement shared by both: when the server is seen to have stopped, a still-pending accept gets a bounded final grace so a connection established just before the stop is not discarded in favor of a misleading "stopped before connecting back" failure. The path is loopback TCP, so it fails fast with `PlatformNotSupportedException` on browser/WASM; it does not enable WASM hosting. Fixes#10890 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Two independent reviews of the in-process launch path raised the same top finding: `Dispose()` waits for the hosted application synchronously on the calling thread, and on the very platforms this feature targets (MAUI, Android, iOS) a multi-second block on the UI thread trips the ANR / watchdog. Add `IMtpServerClient.ShutdownAsync()` (and `IMtpServerHost.ShutdownAsync`) so the same teardown can be awaited instead. `IAsyncDisposable` stays rejected -- netstandard2.0 and net462 would need `Microsoft.Bcl.AsyncInterfaces`, which breaks the package's dependency-free promise -- but rejecting the interface is not a reason to have no asynchronous shutdown at all. `Dispose()` remains, is still idempotent, and returns immediately after `ShutdownAsync`. Remove the accept grace period. The previous commit let a still-pending accept win for ~100ms after the server was seen to have stopped, on the theory that a connection established just before the stop should not be discarded. That theory does not hold: if the server has stopped, the socket belongs to a dead peer, so the grace only traded a precise failure (exit code plus captured stderr) for a generic connection-closed error on the first request, and its uncancellable delay could let the server failure beat a concurrent caller cancellation. Dropping it also makes the external-process path provably unchanged, so the changelog no longer needs a `Changed` entry. Other review fixes: * Expose `IMtpServerClient.ServerExitCode`. The callback signature already demanded a `Task<int>`, but after a successful session the value was unreachable, so an embedded host whose `Main` must return it had to capture it in a closure. * Do not dispose the server's `CancellationTokenSource` when the callback was abandoned while still running: it holds the token, and `token.WaitHandle` or `CreateLinkedTokenSource` would then throw `ObjectDisposedException` inside the caller's own code. * Skip the graceful wait entirely on a failed launch. Nothing is connected, so there is no transport closure for the callback to observe; only the fixed cancellation grace applies and an unwinding caller no longer pays `ServerShutdownTimeout`. * Clamp bounded waits instead of trusting the caller's `TimeSpan`. A negative value (or `Timeout.InfiniteTimeSpan`) made `Task.Delay` throw from a path documented never to throw; an oversized one is capped to the largest delay .NET Framework accepts. * Move `TcpListener.Start()` inside the cleanup `try`: it creates the socket before binding, so a bind failure leaked it because the caller never received a listener it could stop. * Log a late failure from an abandoned callback rather than only observing it. * Cache the in-process host's `ProcessId` instead of allocating a `Process` per property read. * Document that `Dispose()` blocks, that cancellation is bounded rather than immediate, and why the two launch paths still build different argument shapes; fix the shadowed `cancellationToken` in the PACKAGE.md sample. Tests: cover `ShutdownAsync`, `ServerExitCode`, and fail-fast on a server that stops without connecting; make `Dispose_IsIdempotent` able to fail by asserting on observed transport closes rather than callback invocations; replace the hand-rolled throws helper with `Assert.ThrowsExactlyAsync`; tighten the shutdown-bound assertion; and stop leaking `Process` handles. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

A third review round found three ways the teardown contract did not hold up under concurrency. `Dispose()` and `ShutdownAsync()` each had their own `Interlocked.Exchange` guard, so whichever call lost the race returned while teardown was still running -- reporting to its caller that the application had stopped when it had not. Replace both guards with one lazily started, shared teardown task: every entry point now joins the same task, so `Dispose` after (or racing) `ShutdownAsync` returns only once the application has actually stopped, and the whole thing stays idempotent. `MtpServerClient` no longer keeps its own guard either; it delegates to the host, whose teardown is the single source of truth. `ShutdownAsync()` could still block the very thread it exists to protect: it closed the transport before its first await, and closing the connection waits up to five seconds for the read loop. The shared task is started with `Task.Run`, so the whole teardown -- transport close included -- runs on the thread pool and `ShutdownAsync` returns immediately. `MtpServerProcess` likewise now runs its (bounded but synchronous) kill off the calling thread rather than pretending to be async while blocking. `CancellationTokenSource.Cancel()` executes registrations synchronously on the calling thread, so a caller registration that blocked would prevent the five-second cancellation grace from ever starting and make the "bounded" shutdown unbounded. Start the cancellation separately and begin the grace regardless. When a registration is still executing once the callback has finished, the source is reported as unsafe to dispose for the same reason an abandoned callback is: leaking one `CancellationTokenSource` beats a use-after-dispose inside the caller's code. Adds a test that a `Dispose` racing an in-flight `ShutdownAsync` blocks until the shared teardown completes, and that both share one teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

… fix ExitCode A fourth review round found that only the in-process host had been converted to the shared-teardown design. `MtpServerProcess` kept its old early-return guard, so on the external-process path a `Dispose()` that followed or raced `ShutdownAsync()` returned while `SafeKill`'s bounded `WaitForExit` was still running on another thread -- losing exactly the guarantee that wait exists for (a caller may delete the application directory immediately after disposal), and making `await ShutdownAsync()` report that the server had stopped when it had not. That contradicted the comment the previous commit added to `MtpServerClient.Dispose`, which claimed both paths joined an in-flight teardown. Convert `MtpServerProcess` to the same lazily created shared task, so the contract is now uniform across both implementations of `IMtpServerHost`, and reconcile the interface docs, which previously described the opposite rule. Adding real coverage for `ShutdownAsync` on the external-process path then surfaced a genuine bug: `MtpServerProcess.ExitCode` was always `null` after teardown, because a `Process` cannot be queried once disposed. Capture the exit code during teardown instead -- before the kill, so an application that already exited on its own reports its real code rather than the kill's, and before `Process.Dispose()`, after which nothing is readable. Also fixes a race in the in-process test fixture that made the suite flaky on net462: the callback published its `FakeMtpServer` only after its own connect call returned, but the client's accept can complete first, so a test could reach `Value` before the callback had set it. The fixture now exposes a `Connected` signal the launch helper awaits. 6/6 clean net462 runs and 5/5 clean net8.0 runs afterwards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

+ if (serverTask is not null)
+ {
+ // The launch is being abandoned, so skip the graceful wait entirely: there is no connected
+ // transport whose closure could signal the callback, and the caller (often a canceling one) is
+ // waiting on this unwind. A zero graceful timeout goes straight to cancel-then-grace.
+ if (!await ShutdownServerAsync(serverTask, serverCancellation!, TimeSpan.Zero, logger).ConfigureAwait(false))
+ {
+ // The callback is still running and still holds the token; disposing its source now would
+ // turn a clean abandonment into an ObjectDisposedException inside the caller's own code.
+ throw;
+ }
+ }


github-actions · 2026-08-31T19:32:23Z

🧪 Expert test review — PR #10898

Grade	Test	Mutation	Notes	How to improve
B (80–89)	new `MtpServerClientInProcessTests. Dispose_CallbackFaultsDuringShutdown_ DoesNotThrow`	2/3 killed	Reads `server.Completion.Exception` but never asserts on it, so a mutation changing the faulted exception's identity/message survives.	Assert the captured exception's message/type instead of discarding it after touching `.Exception`.
B (80–89)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_HonorsTheStatefulOption`	3/4 killed	Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on `client.Capabilities` is not caught.	Also assert `client.Capabilities.IsStateful` reflects the negotiated round-trip value, not only the sent request.
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_PassesCompleteServerModeArguments`	5/5 killed	Verifies exact ordered arguments, dynamic port and fixed count; strong regression guard for the generated argument array.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_DrivesInitializeDiscoverRunAndExit`	6/6 killed	End-to-end drive through initialize/discover/run/exit with distinct assertions per state transition.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsBeforeConnecting_ PreservesCallbackException`	2/2 killed	Asserts the exact exception instance is preserved as inner exception, not just its type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsAsynchronouslyBeforeConnecting_ PreservesCallbackException`	2/2 killed	Covers the async-throw variant with the same identity assertion as the sync case.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsWithoutConnecting_ FailsFastInsteadOfWaitingOutTheTimeout`	3/3 killed	Checks both the reported exit code in the message and a timing bound guarding against a slow-timeout regression.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsBeforeConnecting_ ReportsExitCode`	2/2 killed	Asserts the specific exit code surfaces in the exception message.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackReturnsNullTask_Fails`	2/2 killed	Asserts the specific inner exception type for a misbehaving callback returning a null task.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_NullCallback_Throws`	1/1 killed	Focused single-assertion guard-clause test using the exact exception type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_AlreadyCanceled_ DoesNotInvokeCallback`	2/2 killed	Verifies both the cancellation exception and, via an interlocked counter, that the callback never ran.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CanceledWhileConnecting_ CancelsTheCallbackToken`	2/2 killed	Confirms both the caller-side cancellation and that the callback's own token observed cancellation.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_ConnectionTimeoutElapses_ FailsWithTimeoutMessage`	3/3 killed	Distinguishes the connection timeout from the deliberately huge shutdown timeout via message and elapsed-time bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_ClosesTransportAndAwaitsTheCallback`	4/4 killed	Checks pre/post completion state, the callback's returned exit code, and the client's reported exit code.	—
A (90–100)	new `MtpServerClientInProcessTests. ShutdownAsync_ClosesTransportAndAwaitsTheCallback_ WithoutBlocking`	4/4 killed	Verifies await-completion, exit code, a fast follow-up Dispose, and single-teardown count together.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_FromANotificationHandler_ DoesNotSelfWaitOnTheReadLoop`	1/1 killed	Targeted timing assertion guards against a specific re-entrancy deadlock regression.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_WhileShutdownAsyncIsInFlight_ WaitsForTheSameTeardown`	3/3 killed	Uses a controlled release gate to prove the racing Dispose genuinely blocks, then confirms a single shared teardown.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_IsIdempotent`	3/3 killed	Confirms exactly one transport close and one completion despite three Dispose calls, plus a fast-return bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_CallbackIgnoresShutdown_ ReturnsWithinTheDocumentedBound`	2/2 killed	Verifies both the documented abandonment time bound and the logged "abandoning" diagnostic.	—
A (90–100)	new `MtpServerClientInProcessTests. RunTestsAsync_Canceled_ SendsCancelRequestToTheHostedApplication`	2/2 killed	Confirms both the client-side cancellation exception and the wire-level cancel notification reaching the fake server.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_MultipleRequestsOnOneSession_ ReuseTheSameConnection`	3/3 killed	Checks keep-alive negotiation, single-connection reuse count, and total request count together.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_IgnoresEnvironmentVariablesAndWarns`	1/1 killed	Asserts the specific warning content naming the ignored option.	—
A (90–100)	new `MtpServerClientInProcessAcceptanceTests. InProcessHost_ DiscoversAndRunsItsOwnMSTestNodes_ WithoutStartingAProcess`	5/5 killed	Real end-to-end embedded-host run asserting build success, exit code, and each marker line the generated app emits.	—
A (90–100)	mod `MtpServerClientAcceptanceTests. DiscoverAndRun_ViaSourcePackageClient_ ReportsExpectedTestNode`	1/1 killed	New lines assert the external-process ShutdownAsync/Dispose share a teardown and report an exit code.	—
A (90–100)	mod `MtpServerClientSourcePackageConsumerTests. HostileConsumer_ CompilesAgainstPackedSource`	N/A	Compile-only guard; added lines correctly extend surface coverage to `ShutdownAsync` and the in-process launch path.	—

_{This advisory comment was generated automatically. Grades are heuristic

and informational — they do not block merging. Suggestions on the Files

changed tab can be applied with one click. Re-run with
/review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · [◷]( · ◷)

github-actions · 2026-08-31T19:32:24Z

🧵 Parallel-safety audit — PR #10898

Parallelization — audited assemblies:

Test assembly	Scope	Workers	Analyzer coverage
`MSTest.Acceptance.IntegrationTests`	`MethodLevel` (default `[assembly: Parallelize(Scope = ExecutionScope.MethodLevel, Workers = 0)]` in `Program.cs`)	CPU count	coverable once the parallel-safety analyzers ship (attribute-based opt-in)
`Microsoft.Testing.Platform.Acceptance.IntegrationTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped
`Microsoft.Testing.Platform.ServerMode.Client.Sources.UnitTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped

This PR did not touch any .runsettings/testconfig.json/Directory.Build.* parallelization config — no assembly's scope changed.

Findings: A (global-state) 0 · B (paths) 0 · C (declaration) 0 · D (over-serialization) 0 — by severity: Critical 0 · High 0 · Warning 0 · Info 0.

No unsafe call sites found in the lines this PR added or modified.

MtpServerClientAcceptanceTests.cs — added lines only call client.ShutdownAsync() / assert ServerExitCode; no process-global state touched.
MtpServerClientSourcePackageConsumerTests.cs — added lines call into an isolated generated test asset (DriveAsync/DriveInProcessAsync); no shared/relative paths, env vars, culture, or console state mutated at the assembly level (Console.WriteLine here writes to the generated app's own redirected output, not the auditing process).
MtpServerClientInProcessAcceptanceTests.cs (new) — uses TestAsset.GenerateAssetAsync, which allocates a unique GUID-suffixed TempDirectory per instance (TestAsset.cs:19-26, throws if a path already exists) — no shared-path collision. CreateChildEnvironment() builds a fresh Dictionary passed only to the spawned child process's environment, not Environment.SetEnvironmentVariable, so it does not mutate this process's state.
FakeMtpServer.cs / MtpServerClientInProcessTests.cs (new) — each test constructs its own TcpListener/FakeMtpServer bound to an OS-assigned loopback port (new TcpListener(IPAddress.Loopback, 0)), so there is no fixed-port collision between concurrently-running tests; no static/process-global state is written.

Nothing here implies a testability or smell concern beyond this audit's scope.

_{Advisory only — heuristic, non-blocking. Re-run with /parallel-audit. This audit answers "is it parallel-safe?"; for testability, smells, or flakiness see the detect-static-dependencies / test-smell-detection / test-anti-patterns analyses.}

🤖 Automated content by GitHub Copilot. Generated by the Parallel-safety audit on PR (on open / sync) workflow. · auto · 117.1 AIC · ⌖ 3.53 AIC · ⊞ 24.8K · [◷]( · ◷)

github-actions

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · ◷

github-actions · 2026-08-31T19:32:25Z

+ client.Dispose();
+
+ Assert.IsTrue(server.Completion.IsFaulted);
+ _ = server.Completion.Exception;


🧪 Test review · Grade B (80–89) — Reads server.Completion.Exception but never asserts on it, so a mutation changing the faulted exception's identity/message would survive.
Assert the captured exception's message/type instead of discarding it after touching .Exception.
Suggested change
_=server.Completion.Exception;
Exception?faultException=server.Completion.Exception?.GetBaseException();
Assert.IsInstanceOfType<InvalidOperationException>(faultException);
Assert.AreEqual("The application failed while shutting down.",faultException!.Message);

github-actions · 2026-08-31T19:32:25Z

+ ? typed
+ : SerializerUtilities.Deserialize<InitializeRequestArgs>((IDictionary<string, object?>)initialize.Params!);
+
+ Assert.AreEqual(isStateful, args.Capabilities.IsStateful, "The in-process path must forward the client's stateful capability unchanged.");


🧪 Test review · Grade B (80–89) — Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on client.Capabilities is not caught.
Also assert client.Capabilities.IsStateful reflects the negotiated round-trip value, not only the sent request.
Suggested change
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,capabilities.IsStateful,"The negotiated capability the client stores must reflect the server's round trip, not just the sent request.");

github-actions

Note

🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

✅ 22/22 dimensions clean — no findings.

Summary: This PR adds MtpServerClient.LaunchInProcessAsync for embedded hosts (MAUI, Android/iOS) that cannot Process.Start. The implementation is thorough and well-structured:

Threading & Concurrency: Shutdown is idempotent via lock + lazy Task, Volatile.Read/Write for cross-thread exit code, Task.Run avoids sync-context deadlocks. No shared mutable state without synchronization.
Security & IPC: Loopback TCP only, no command-line injection surface (arguments are array-based), WASM fails fast with PlatformNotSupportedException.
Public API: All new API is internal (source-only package). No init accessors. No PublicAPI.Unshipped.txt changes needed.
Performance: GetCurrentProcessId() snapshotted once in constructor. No hot-path allocations. WaitBoundedAsync clamps timeouts correctly.
Cross-TFM: #if NET8_0_OR_GREATER guard on AcceptTcpClientAsync(CancellationToken). RuntimeInformation used for OS detection on net462.
Resource Management: Every disposable (TcpListener, TcpClient, CancellationTokenSource, Process) has cleanup in both success and error paths. Pending accepts are neutralized. ServerCancellation is only disposed when safe.
Defensive Coding: Callback exceptions are wrapped as InnerException in MtpServerConnectionClosedException. Null task from callback is caught. Cancel() runs on thread pool to avoid blocking registration.
Error Handling: Shared teardown task wrapped in catch-all so it never faults. ObserveFailure prevents UnobservedTaskException. Every teardown helper is non-throwing.
Tests: 68 unit tests + acceptance tests covering argument shape, lifecycle, faults, cancellation, timeouts, idempotent disposal, notification-handler disposal, and the real end-to-end in-process path.
Documentation: Changelog, protocol intro, PACKAGE.md all updated. XML doc comments are thorough.

The refactoring of shared transport logic into MtpServerConnector and the IMtpServerHost abstraction is clean and prevents the two launch paths from drifting. The existing LaunchAsync(path) path is provably unchanged (same argument string, same failure messages, same teardown order).

Copilot

Copilot review overview

Review tier: Balanced
Findings: 3

New issues introduced by this change (3)

Severity	Finding
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `MtpJsonRpcConnection.Dispose()` closes the socket and then waits up to its 5-second read-loop…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `ShutdownAsync` writes this nullable property from the teardown worker while callers can read…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — A canceled callback takes this branch without an inner exception, although the package…

What changed in this PR

Adds in-process launch support to the source-only MTP server-mode client for embedded/mobile hosts, addressing #10890 and enabling scenarios related to #9809.

Changes:

Adds callback-based in-process hosting with shared transport setup.
Adds bounded asynchronous shutdown and server exit-code reporting.
Adds documentation, unit coverage, and real-MSTest acceptance coverage.

File	Description
`MtpServerClientInProcessTests.cs`	Tests in-process lifecycle and protocol behavior.
`FakeMtpServer.cs`	Supports server-to-client dial-back mode.
`MtpServerClientInProcessAcceptanceTests.cs`	Exercises a real in-process MSTest application.
`MtpServerClientAcceptanceTests.cs`	Covers external-process shutdown and exit code.
`MtpServerClientSourcePackageConsumerTests.cs`	Extends source-package compile coverage.
`PACKAGE.md`	Documents embedded-host usage and lifecycle.
`MtpServerProcess.cs`	Adopts shared host and shutdown abstractions.
`MtpServerInProcessHost.cs`	Implements callback hosting and teardown.
`MtpServerConnector.cs`	Centralizes listener and transport setup.
`MtpServerClientOptions.cs`	Adds the shutdown timeout option.
`MtpServerClientExceptions.cs`	Supports preserved inner exceptions.
`MtpServerClient.cs`	Exposes in-process launch and shutdown.
`IMtpServerHost.cs`	Defines common host ownership behavior.
`IMtpServerClient.cs`	Adds shutdown and exit-code members.
`001-protocol-intro.md`	Documents reference-client launch modes.
`Changelog-Platform.md`	Records the new embedded-host capability.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+ Connection.Dispose();
+ SafeDispose(_client, _logger, "Disposing the accepted client socket");
+ MtpServerConnector.SafeStop(_listener, _logger);
+
+ bool stopped = await ShutdownServerAsync(_serverTask, _serverCancellation, _shutdownTimeout, _logger).ConfigureAwait(false);


+ /// Gets the exit code the hosted application returned, or <see langword="null"/> while it is still
+ /// running (or when it failed or was abandoned rather than returning one).
+ /// </summary>
+ public int? ExitCode { get; private set; }


+ if (serverTask.IsCanceled)
+ {
+ return new MtpServerConnectionClosedException(
+ "The in-process Microsoft.Testing.Platform application was canceled before connecting back.");
+ }


Amaury Levé (Evangelink) · 2026-08-31T19:22:25Z

Fixes#10890

Why

MtpServerClient.LaunchAsync(path) owns the loopback listener and starts the test application with Process.Start. That is right for IDE and desktop tooling, but unusable for embedded hosts — MAUI apps, Android/iOS test apps — where the MTP application already runs in the caller's process.

Today those hosts have to reimplement the listener, the --server/host/port arguments, the race between connection and startup failure, the serializer-before-formatter registration order, TcpMessageHandler + MtpJsonRpcConnection construction, and the exit / transport-close / server-completion shutdown dance. That defeats the point of shipping a canonical client and makes it easy to reintroduce bugs this package already solved: partial frame writes, ignored JSON-RPC errors, missing $/cancelRequest, unbounded waits, exception masking.

A concrete consumer is the DeviceRunners MSTest visual-runner work (mattleibow/DeviceRunners#157, #9809).

What

usingIMtpServerClientclient=awaitMtpServerClient.LaunchInProcessAsync(async(serverArgs,token)=>{ITestApplicationBuilderbuilder=awaitTestApplication.CreateBuilderAsync(serverArgs);builder.AddMSTest(()=>testAssemblies);usingITestApplicationapp=awaitbuilder.BuildAsync();returnawaitapp.RunAsync();},options,cancellationToken);awaitclient.InitializeAsync();awaitclient.DiscoverTestsAsync();awaitclient.RunTestsAsync();awaitclient.ExitAsync();awaitclient.ShutdownAsync();

The caller supplies only "how to run the application". The client keeps everything else: it binds the listener, generates the complete argument array (--server jsonrpc --client-host 127.0.0.1 --client-port <port> --no-banner), races the connect, builds the transport, and owns a bounded shutdown.

New API on the injected (still internal) surface:

Member	Purpose
`MtpServerClient.LaunchInProcessAsync(callback, options, ct)`	The embedded-host launch path. Async only — blocking the launching thread can deadlock the application being launched.
`IMtpServerClient.ShutdownAsync()`	Non-blocking teardown.
`IMtpServerClient.ServerExitCode`	The value the application returned.
`MtpServerClientOptions.ServerShutdownTimeout`	Graceful shutdown bound (default 30s).

Internally, the transport setup was extracted from MtpServerProcess into MtpServerConnector, and both launch paths now sit behind IMtpServerHost, so they cannot drift.

Behavior worth reviewing

Ownership and shutdown. Both Dispose() and ShutdownAsync() join one lazily created shared teardown task, on both hosts. A Dispose that follows or races ShutdownAsync therefore returns only once the server has actually stopped, rather than reporting success while teardown is still running. Teardown runs on the thread pool, so ShutdownAsync never blocks and the synchronous Dispose cannot deadlock against a UI-thread continuation. Task.Run captures the execution context, so the connection's read-loop AsyncLocal marker still flows and disposing from a notification handler does not self-wait (covered by a test asserting < 4s against the connection's 5s read-loop timeout).

Bounded, and actually bounded. Teardown waits ServerShutdownTimeout, then cancels the callback's token, then a fixed 5s grace, then abandons and logs. CancellationTokenSource.Cancel() runs registrations synchronously, so the cancellation is started separately — otherwise a blocking caller registration would prevent the grace from ever starting and make the "bounded" wait unbounded. A failed launch skips the graceful wait entirely: nothing is connected, so there is no transport closure for the callback to observe.

Exception preservation. A callback that throws, is canceled, or returns before dialing back surfaces as MtpServerConnectionClosedException with the original as InnerException, instead of a misleading connection timeout. Every teardown helper is non-throwing, and the shared teardown task is wrapped so it can never fault — a faulted shared task would throw from every later disposal.

LaunchAsync(path) is unchanged. Verified against main: identical argument string, failure messages, stderr capture, exit fast-fail and teardown order. One earlier revision of this branch added a grace period to the accept race; review showed it only traded a precise failure for a vague one, so it was removed and the external path is now provably unchanged.

Browser/WASM. Both paths are loopback TCP. LaunchInProcessAsync fails fast with PlatformNotSupportedException there; this does not enable WASM hosting, and the docs say so.

Tests

68 unit tests (net8.0) / 63 (net462, Jsonite path). New coverage: argument array shape; initialize → discover → run → exit; callback faulting synchronously and asynchronously; callback exiting with a code; null task; pre-canceled launch not invoking the callback; cancellation during connect canceling the callback token; connection timeout bounded by the grace rather than ServerShutdownTimeout; ShutdownAsync; ServerExitCode; disposal awaiting the callback; Dispose racing an in-flight ShutdownAsync; Dispose from a notification handler; idempotent disposal; callback faulting during shutdown; unresponsive callback abandoned within the bound; $/cancelRequest; stateful on/off; multi-request single connection; EnvironmentVariables ignored and warned. Stressed 6×/5× consecutively for flakiness.
New acceptance test where a single generated process is simultaneously the embedded host (compiling the packed source-only package) and a real MTP TestApplication with real MSTest over a real [TestClass] — discovering and running its own test over JSON-RPC with no Process.Start anywhere. It also asserts the server's reported process id equals its own.
Existing external-process acceptance test extended to cover ShutdownAsync + ServerExitCode; hostile-consumer compile oracle extended to bind the new API on net462/netstandard2.0/net5.0–net8.0.
Full regression: 13 platform source-package/consumer acceptance, 4 MSTest acceptance, 13 platform ServerTests. build.cmd -pack clean, 0 warnings.

Review history

Four independent review rounds (MTP/MSTest expert reviewer, a design reviewer, and two correctness reviewers) produced 3 major, 10 moderate and 10 minor findings, all addressed. Several were real bugs the tests then locked in — notably Process.ExitCode being unreadable after Process.Dispose(), a TcpListener socket leak when Start() failed, and a teardown path that could throw from a contract documented never to throw.

Open design questions

No IAsyncDisposable. netstandard2.0/net462 would need Microsoft.Bcl.AsyncInterfaces, breaking the package's dependency-free promise. ShutdownAsync() is the substitute. Happy to revisit if the dependency is acceptable.
MtpServerClientOptions is mode-mixed.EnvironmentVariables is external-process only; ServerShutdownTimeout is in-process only. Nesting per-transport options would age better, but EnvironmentVariables already shipped at the top level, so it cannot be done non-breakingly now.
The 5s cancellation grace is a fixed constant, not an option.
The two paths still build different argument shapes — deliberate, to keep the shipped external command line byte-identical. Commented at the call site.
No ConnectAsync(TcpClient/Stream, options) factory — the existing MtpServerClient(MtpJsonRpcConnection, options) constructor already covers "wrap an existing transport", and the guidance is to minimize injected surface. Easy to add if wanted.

Embedded hosts such as MAUI or Android/iOS test apps cannot spawn a child process, so `MtpServerClient.LaunchAsync(path)` was unusable for them and they had to reimplement the listener, the server-mode arguments, the connect race, the serializer/formatter ordering, the transport and the shutdown coordination by hand -- reintroducing the bugs the canonical client already solves. Add `MtpServerClient.LaunchInProcessAsync(callback, options, cancellationToken)`. The client keeps ownership of everything except "how to run the application": it binds the loopback listener, generates the complete server-mode argument array, races the connect against callback failure/completion, caller cancellation and the connection timeout, and owns a bounded shutdown that closes the transport, then cancels the callback token, then abandons it rather than hanging the caller. A callback failure before connection is surfaced as `MtpServerConnectionClosedException` with the caller's exception preserved as the inner exception, and teardown failures are only logged so they can never replace the primary failure. The shared transport setup is extracted from `MtpServerProcess` into `MtpServerConnector` and both launch paths now flow through `IMtpServerHost`, so the external-process behavior is unchanged while the two paths cannot drift. The one behavior change is an improvement shared by both: when the server is seen to have stopped, a still-pending accept gets a bounded final grace so a connection established just before the stop is not discarded in favor of a misleading "stopped before connecting back" failure. The path is loopback TCP, so it fails fast with `PlatformNotSupportedException` on browser/WASM; it does not enable WASM hosting. Fixes#10890 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Two independent reviews of the in-process launch path raised the same top finding: `Dispose()` waits for the hosted application synchronously on the calling thread, and on the very platforms this feature targets (MAUI, Android, iOS) a multi-second block on the UI thread trips the ANR / watchdog. Add `IMtpServerClient.ShutdownAsync()` (and `IMtpServerHost.ShutdownAsync`) so the same teardown can be awaited instead. `IAsyncDisposable` stays rejected -- netstandard2.0 and net462 would need `Microsoft.Bcl.AsyncInterfaces`, which breaks the package's dependency-free promise -- but rejecting the interface is not a reason to have no asynchronous shutdown at all. `Dispose()` remains, is still idempotent, and returns immediately after `ShutdownAsync`. Remove the accept grace period. The previous commit let a still-pending accept win for ~100ms after the server was seen to have stopped, on the theory that a connection established just before the stop should not be discarded. That theory does not hold: if the server has stopped, the socket belongs to a dead peer, so the grace only traded a precise failure (exit code plus captured stderr) for a generic connection-closed error on the first request, and its uncancellable delay could let the server failure beat a concurrent caller cancellation. Dropping it also makes the external-process path provably unchanged, so the changelog no longer needs a `Changed` entry. Other review fixes: * Expose `IMtpServerClient.ServerExitCode`. The callback signature already demanded a `Task<int>`, but after a successful session the value was unreachable, so an embedded host whose `Main` must return it had to capture it in a closure. * Do not dispose the server's `CancellationTokenSource` when the callback was abandoned while still running: it holds the token, and `token.WaitHandle` or `CreateLinkedTokenSource` would then throw `ObjectDisposedException` inside the caller's own code. * Skip the graceful wait entirely on a failed launch. Nothing is connected, so there is no transport closure for the callback to observe; only the fixed cancellation grace applies and an unwinding caller no longer pays `ServerShutdownTimeout`. * Clamp bounded waits instead of trusting the caller's `TimeSpan`. A negative value (or `Timeout.InfiniteTimeSpan`) made `Task.Delay` throw from a path documented never to throw; an oversized one is capped to the largest delay .NET Framework accepts. * Move `TcpListener.Start()` inside the cleanup `try`: it creates the socket before binding, so a bind failure leaked it because the caller never received a listener it could stop. * Log a late failure from an abandoned callback rather than only observing it. * Cache the in-process host's `ProcessId` instead of allocating a `Process` per property read. * Document that `Dispose()` blocks, that cancellation is bounded rather than immediate, and why the two launch paths still build different argument shapes; fix the shadowed `cancellationToken` in the PACKAGE.md sample. Tests: cover `ShutdownAsync`, `ServerExitCode`, and fail-fast on a server that stops without connecting; make `Dispose_IsIdempotent` able to fail by asserting on observed transport closes rather than callback invocations; replace the hand-rolled throws helper with `Assert.ThrowsExactlyAsync`; tighten the shutdown-bound assertion; and stop leaking `Process` handles. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

A third review round found three ways the teardown contract did not hold up under concurrency. `Dispose()` and `ShutdownAsync()` each had their own `Interlocked.Exchange` guard, so whichever call lost the race returned while teardown was still running -- reporting to its caller that the application had stopped when it had not. Replace both guards with one lazily started, shared teardown task: every entry point now joins the same task, so `Dispose` after (or racing) `ShutdownAsync` returns only once the application has actually stopped, and the whole thing stays idempotent. `MtpServerClient` no longer keeps its own guard either; it delegates to the host, whose teardown is the single source of truth. `ShutdownAsync()` could still block the very thread it exists to protect: it closed the transport before its first await, and closing the connection waits up to five seconds for the read loop. The shared task is started with `Task.Run`, so the whole teardown -- transport close included -- runs on the thread pool and `ShutdownAsync` returns immediately. `MtpServerProcess` likewise now runs its (bounded but synchronous) kill off the calling thread rather than pretending to be async while blocking. `CancellationTokenSource.Cancel()` executes registrations synchronously on the calling thread, so a caller registration that blocked would prevent the five-second cancellation grace from ever starting and make the "bounded" shutdown unbounded. Start the cancellation separately and begin the grace regardless. When a registration is still executing once the callback has finished, the source is reported as unsafe to dispose for the same reason an abandoned callback is: leaking one `CancellationTokenSource` beats a use-after-dispose inside the caller's code. Adds a test that a `Dispose` racing an in-flight `ShutdownAsync` blocks until the shared teardown completes, and that both share one teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

… fix ExitCode A fourth review round found that only the in-process host had been converted to the shared-teardown design. `MtpServerProcess` kept its old early-return guard, so on the external-process path a `Dispose()` that followed or raced `ShutdownAsync()` returned while `SafeKill`'s bounded `WaitForExit` was still running on another thread -- losing exactly the guarantee that wait exists for (a caller may delete the application directory immediately after disposal), and making `await ShutdownAsync()` report that the server had stopped when it had not. That contradicted the comment the previous commit added to `MtpServerClient.Dispose`, which claimed both paths joined an in-flight teardown. Convert `MtpServerProcess` to the same lazily created shared task, so the contract is now uniform across both implementations of `IMtpServerHost`, and reconcile the interface docs, which previously described the opposite rule. Adding real coverage for `ShutdownAsync` on the external-process path then surfaced a genuine bug: `MtpServerProcess.ExitCode` was always `null` after teardown, because a `Process` cannot be queried once disposed. Capture the exit code during teardown instead -- before the kill, so an application that already exited on its own reports its real code rather than the kill's, and before `Process.Dispose()`, after which nothing is readable. Also fixes a race in the in-process test fixture that made the suite flaky on net462: the callback published its `FakeMtpServer` only after its own connect call returned, but the client's accept can complete first, so a test could reach `Value` before the callback had set it. The fixture now exposes a `Connected` signal the launch helper awaits. 6/6 clean net462 runs and 5/5 clean net8.0 runs afterwards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

+ if (serverTask is not null)
+ {
+ // The launch is being abandoned, so skip the graceful wait entirely: there is no connected
+ // transport whose closure could signal the callback, and the caller (often a canceling one) is
+ // waiting on this unwind. A zero graceful timeout goes straight to cancel-then-grace.
+ if (!await ShutdownServerAsync(serverTask, serverCancellation!, TimeSpan.Zero, logger).ConfigureAwait(false))
+ {
+ // The callback is still running and still holds the token; disposing its source now would
+ // turn a clean abandonment into an ObjectDisposedException inside the caller's own code.
+ throw;
+ }
+ }


github-actions · 2026-08-31T19:32:23Z

🧪 Expert test review — PR #10898

Grade	Test	Mutation	Notes	How to improve
B (80–89)	new `MtpServerClientInProcessTests. Dispose_CallbackFaultsDuringShutdown_ DoesNotThrow`	2/3 killed	Reads `server.Completion.Exception` but never asserts on it, so a mutation changing the faulted exception's identity/message survives.	Assert the captured exception's message/type instead of discarding it after touching `.Exception`.
B (80–89)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_HonorsTheStatefulOption`	3/4 killed	Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on `client.Capabilities` is not caught.	Also assert `client.Capabilities.IsStateful` reflects the negotiated round-trip value, not only the sent request.
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_PassesCompleteServerModeArguments`	5/5 killed	Verifies exact ordered arguments, dynamic port and fixed count; strong regression guard for the generated argument array.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_DrivesInitializeDiscoverRunAndExit`	6/6 killed	End-to-end drive through initialize/discover/run/exit with distinct assertions per state transition.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsBeforeConnecting_ PreservesCallbackException`	2/2 killed	Asserts the exact exception instance is preserved as inner exception, not just its type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsAsynchronouslyBeforeConnecting_ PreservesCallbackException`	2/2 killed	Covers the async-throw variant with the same identity assertion as the sync case.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsWithoutConnecting_ FailsFastInsteadOfWaitingOutTheTimeout`	3/3 killed	Checks both the reported exit code in the message and a timing bound guarding against a slow-timeout regression.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsBeforeConnecting_ ReportsExitCode`	2/2 killed	Asserts the specific exit code surfaces in the exception message.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackReturnsNullTask_Fails`	2/2 killed	Asserts the specific inner exception type for a misbehaving callback returning a null task.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_NullCallback_Throws`	1/1 killed	Focused single-assertion guard-clause test using the exact exception type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_AlreadyCanceled_ DoesNotInvokeCallback`	2/2 killed	Verifies both the cancellation exception and, via an interlocked counter, that the callback never ran.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CanceledWhileConnecting_ CancelsTheCallbackToken`	2/2 killed	Confirms both the caller-side cancellation and that the callback's own token observed cancellation.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_ConnectionTimeoutElapses_ FailsWithTimeoutMessage`	3/3 killed	Distinguishes the connection timeout from the deliberately huge shutdown timeout via message and elapsed-time bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_ClosesTransportAndAwaitsTheCallback`	4/4 killed	Checks pre/post completion state, the callback's returned exit code, and the client's reported exit code.	—
A (90–100)	new `MtpServerClientInProcessTests. ShutdownAsync_ClosesTransportAndAwaitsTheCallback_ WithoutBlocking`	4/4 killed	Verifies await-completion, exit code, a fast follow-up Dispose, and single-teardown count together.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_FromANotificationHandler_ DoesNotSelfWaitOnTheReadLoop`	1/1 killed	Targeted timing assertion guards against a specific re-entrancy deadlock regression.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_WhileShutdownAsyncIsInFlight_ WaitsForTheSameTeardown`	3/3 killed	Uses a controlled release gate to prove the racing Dispose genuinely blocks, then confirms a single shared teardown.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_IsIdempotent`	3/3 killed	Confirms exactly one transport close and one completion despite three Dispose calls, plus a fast-return bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_CallbackIgnoresShutdown_ ReturnsWithinTheDocumentedBound`	2/2 killed	Verifies both the documented abandonment time bound and the logged "abandoning" diagnostic.	—
A (90–100)	new `MtpServerClientInProcessTests. RunTestsAsync_Canceled_ SendsCancelRequestToTheHostedApplication`	2/2 killed	Confirms both the client-side cancellation exception and the wire-level cancel notification reaching the fake server.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_MultipleRequestsOnOneSession_ ReuseTheSameConnection`	3/3 killed	Checks keep-alive negotiation, single-connection reuse count, and total request count together.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_IgnoresEnvironmentVariablesAndWarns`	1/1 killed	Asserts the specific warning content naming the ignored option.	—
A (90–100)	new `MtpServerClientInProcessAcceptanceTests. InProcessHost_ DiscoversAndRunsItsOwnMSTestNodes_ WithoutStartingAProcess`	5/5 killed	Real end-to-end embedded-host run asserting build success, exit code, and each marker line the generated app emits.	—
A (90–100)	mod `MtpServerClientAcceptanceTests. DiscoverAndRun_ViaSourcePackageClient_ ReportsExpectedTestNode`	1/1 killed	New lines assert the external-process ShutdownAsync/Dispose share a teardown and report an exit code.	—
A (90–100)	mod `MtpServerClientSourcePackageConsumerTests. HostileConsumer_ CompilesAgainstPackedSource`	N/A	Compile-only guard; added lines correctly extend surface coverage to `ShutdownAsync` and the in-process launch path.	—

_{This advisory comment was generated automatically. Grades are heuristic

and informational — they do not block merging. Suggestions on the Files

changed tab can be applied with one click. Re-run with
/review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · [◷]( · ◷)

github-actions · 2026-08-31T19:32:24Z

🧵 Parallel-safety audit — PR #10898

Parallelization — audited assemblies:

Test assembly	Scope	Workers	Analyzer coverage
`MSTest.Acceptance.IntegrationTests`	`MethodLevel` (default `[assembly: Parallelize(Scope = ExecutionScope.MethodLevel, Workers = 0)]` in `Program.cs`)	CPU count	coverable once the parallel-safety analyzers ship (attribute-based opt-in)
`Microsoft.Testing.Platform.Acceptance.IntegrationTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped
`Microsoft.Testing.Platform.ServerMode.Client.Sources.UnitTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped

This PR did not touch any .runsettings/testconfig.json/Directory.Build.* parallelization config — no assembly's scope changed.

Findings: A (global-state) 0 · B (paths) 0 · C (declaration) 0 · D (over-serialization) 0 — by severity: Critical 0 · High 0 · Warning 0 · Info 0.

No unsafe call sites found in the lines this PR added or modified.

MtpServerClientAcceptanceTests.cs — added lines only call client.ShutdownAsync() / assert ServerExitCode; no process-global state touched.
MtpServerClientSourcePackageConsumerTests.cs — added lines call into an isolated generated test asset (DriveAsync/DriveInProcessAsync); no shared/relative paths, env vars, culture, or console state mutated at the assembly level (Console.WriteLine here writes to the generated app's own redirected output, not the auditing process).
MtpServerClientInProcessAcceptanceTests.cs (new) — uses TestAsset.GenerateAssetAsync, which allocates a unique GUID-suffixed TempDirectory per instance (TestAsset.cs:19-26, throws if a path already exists) — no shared-path collision. CreateChildEnvironment() builds a fresh Dictionary passed only to the spawned child process's environment, not Environment.SetEnvironmentVariable, so it does not mutate this process's state.
FakeMtpServer.cs / MtpServerClientInProcessTests.cs (new) — each test constructs its own TcpListener/FakeMtpServer bound to an OS-assigned loopback port (new TcpListener(IPAddress.Loopback, 0)), so there is no fixed-port collision between concurrently-running tests; no static/process-global state is written.

Nothing here implies a testability or smell concern beyond this audit's scope.

_{Advisory only — heuristic, non-blocking. Re-run with /parallel-audit. This audit answers "is it parallel-safe?"; for testability, smells, or flakiness see the detect-static-dependencies / test-smell-detection / test-anti-patterns analyses.}

🤖 Automated content by GitHub Copilot. Generated by the Parallel-safety audit on PR (on open / sync) workflow. · auto · 117.1 AIC · ⌖ 3.53 AIC · ⊞ 24.8K · [◷]( · ◷)

github-actions

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · ◷

github-actions · 2026-08-31T19:32:25Z

+ client.Dispose();
+
+ Assert.IsTrue(server.Completion.IsFaulted);
+ _ = server.Completion.Exception;


🧪 Test review · Grade B (80–89) — Reads server.Completion.Exception but never asserts on it, so a mutation changing the faulted exception's identity/message would survive.
Assert the captured exception's message/type instead of discarding it after touching .Exception.
Suggested change
_=server.Completion.Exception;
Exception?faultException=server.Completion.Exception?.GetBaseException();
Assert.IsInstanceOfType<InvalidOperationException>(faultException);
Assert.AreEqual("The application failed while shutting down.",faultException!.Message);

github-actions · 2026-08-31T19:32:25Z

+ ? typed
+ : SerializerUtilities.Deserialize<InitializeRequestArgs>((IDictionary<string, object?>)initialize.Params!);
+
+ Assert.AreEqual(isStateful, args.Capabilities.IsStateful, "The in-process path must forward the client's stateful capability unchanged.");


🧪 Test review · Grade B (80–89) — Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on client.Capabilities is not caught.
Also assert client.Capabilities.IsStateful reflects the negotiated round-trip value, not only the sent request.
Suggested change
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,capabilities.IsStateful,"The negotiated capability the client stores must reflect the server's round trip, not just the sent request.");

github-actions

Note

🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

✅ 22/22 dimensions clean — no findings.

Summary: This PR adds MtpServerClient.LaunchInProcessAsync for embedded hosts (MAUI, Android/iOS) that cannot Process.Start. The implementation is thorough and well-structured:

Threading & Concurrency: Shutdown is idempotent via lock + lazy Task, Volatile.Read/Write for cross-thread exit code, Task.Run avoids sync-context deadlocks. No shared mutable state without synchronization.
Security & IPC: Loopback TCP only, no command-line injection surface (arguments are array-based), WASM fails fast with PlatformNotSupportedException.
Public API: All new API is internal (source-only package). No init accessors. No PublicAPI.Unshipped.txt changes needed.
Performance: GetCurrentProcessId() snapshotted once in constructor. No hot-path allocations. WaitBoundedAsync clamps timeouts correctly.
Cross-TFM: #if NET8_0_OR_GREATER guard on AcceptTcpClientAsync(CancellationToken). RuntimeInformation used for OS detection on net462.
Resource Management: Every disposable (TcpListener, TcpClient, CancellationTokenSource, Process) has cleanup in both success and error paths. Pending accepts are neutralized. ServerCancellation is only disposed when safe.
Defensive Coding: Callback exceptions are wrapped as InnerException in MtpServerConnectionClosedException. Null task from callback is caught. Cancel() runs on thread pool to avoid blocking registration.
Error Handling: Shared teardown task wrapped in catch-all so it never faults. ObserveFailure prevents UnobservedTaskException. Every teardown helper is non-throwing.
Tests: 68 unit tests + acceptance tests covering argument shape, lifecycle, faults, cancellation, timeouts, idempotent disposal, notification-handler disposal, and the real end-to-end in-process path.
Documentation: Changelog, protocol intro, PACKAGE.md all updated. XML doc comments are thorough.

The refactoring of shared transport logic into MtpServerConnector and the IMtpServerHost abstraction is clean and prevents the two launch paths from drifting. The existing LaunchAsync(path) path is provably unchanged (same argument string, same failure messages, same teardown order).

Copilot

Copilot review overview

Review tier: Balanced
Findings: 3

New issues introduced by this change (3)

Severity	Finding
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `MtpJsonRpcConnection.Dispose()` closes the socket and then waits up to its 5-second read-loop…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `ShutdownAsync` writes this nullable property from the teardown worker while callers can read…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — A canceled callback takes this branch without an inner exception, although the package…

What changed in this PR

Adds in-process launch support to the source-only MTP server-mode client for embedded/mobile hosts, addressing #10890 and enabling scenarios related to #9809.

Changes:

Adds callback-based in-process hosting with shared transport setup.
Adds bounded asynchronous shutdown and server exit-code reporting.
Adds documentation, unit coverage, and real-MSTest acceptance coverage.

File	Description
`MtpServerClientInProcessTests.cs`	Tests in-process lifecycle and protocol behavior.
`FakeMtpServer.cs`	Supports server-to-client dial-back mode.
`MtpServerClientInProcessAcceptanceTests.cs`	Exercises a real in-process MSTest application.
`MtpServerClientAcceptanceTests.cs`	Covers external-process shutdown and exit code.
`MtpServerClientSourcePackageConsumerTests.cs`	Extends source-package compile coverage.
`PACKAGE.md`	Documents embedded-host usage and lifecycle.
`MtpServerProcess.cs`	Adopts shared host and shutdown abstractions.
`MtpServerInProcessHost.cs`	Implements callback hosting and teardown.
`MtpServerConnector.cs`	Centralizes listener and transport setup.
`MtpServerClientOptions.cs`	Adds the shutdown timeout option.
`MtpServerClientExceptions.cs`	Supports preserved inner exceptions.
`MtpServerClient.cs`	Exposes in-process launch and shutdown.
`IMtpServerHost.cs`	Defines common host ownership behavior.
`IMtpServerClient.cs`	Adds shutdown and exit-code members.
`001-protocol-intro.md`	Documents reference-client launch modes.
`Changelog-Platform.md`	Records the new embedded-host capability.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+ Connection.Dispose();
+ SafeDispose(_client, _logger, "Disposing the accepted client socket");
+ MtpServerConnector.SafeStop(_listener, _logger);
+
+ bool stopped = await ShutdownServerAsync(_serverTask, _serverCancellation, _shutdownTimeout, _logger).ConfigureAwait(false);


+ /// Gets the exit code the hosted application returned, or <see langword="null"/> while it is still
+ /// running (or when it failed or was abandoned rather than returning one).
+ /// </summary>
+ public int? ExitCode { get; private set; }


+ if (serverTask.IsCanceled)
+ {
+ return new MtpServerConnectionClosedException(
+ "The in-process Microsoft.Testing.Platform application was canceled before connecting back.");
+ }


Amaury Levé (Evangelink) · 2026-08-31T19:22:25Z

Fixes#10890

Why

MtpServerClient.LaunchAsync(path) owns the loopback listener and starts the test application with Process.Start. That is right for IDE and desktop tooling, but unusable for embedded hosts — MAUI apps, Android/iOS test apps — where the MTP application already runs in the caller's process.

Today those hosts have to reimplement the listener, the --server/host/port arguments, the race between connection and startup failure, the serializer-before-formatter registration order, TcpMessageHandler + MtpJsonRpcConnection construction, and the exit / transport-close / server-completion shutdown dance. That defeats the point of shipping a canonical client and makes it easy to reintroduce bugs this package already solved: partial frame writes, ignored JSON-RPC errors, missing $/cancelRequest, unbounded waits, exception masking.

A concrete consumer is the DeviceRunners MSTest visual-runner work (mattleibow/DeviceRunners#157, #9809).

What

usingIMtpServerClientclient=awaitMtpServerClient.LaunchInProcessAsync(async(serverArgs,token)=>{ITestApplicationBuilderbuilder=awaitTestApplication.CreateBuilderAsync(serverArgs);builder.AddMSTest(()=>testAssemblies);usingITestApplicationapp=awaitbuilder.BuildAsync();returnawaitapp.RunAsync();},options,cancellationToken);awaitclient.InitializeAsync();awaitclient.DiscoverTestsAsync();awaitclient.RunTestsAsync();awaitclient.ExitAsync();awaitclient.ShutdownAsync();

The caller supplies only "how to run the application". The client keeps everything else: it binds the listener, generates the complete argument array (--server jsonrpc --client-host 127.0.0.1 --client-port <port> --no-banner), races the connect, builds the transport, and owns a bounded shutdown.

New API on the injected (still internal) surface:

Member	Purpose
`MtpServerClient.LaunchInProcessAsync(callback, options, ct)`	The embedded-host launch path. Async only — blocking the launching thread can deadlock the application being launched.
`IMtpServerClient.ShutdownAsync()`	Non-blocking teardown.
`IMtpServerClient.ServerExitCode`	The value the application returned.
`MtpServerClientOptions.ServerShutdownTimeout`	Graceful shutdown bound (default 30s).

Internally, the transport setup was extracted from MtpServerProcess into MtpServerConnector, and both launch paths now sit behind IMtpServerHost, so they cannot drift.

Behavior worth reviewing

Ownership and shutdown. Both Dispose() and ShutdownAsync() join one lazily created shared teardown task, on both hosts. A Dispose that follows or races ShutdownAsync therefore returns only once the server has actually stopped, rather than reporting success while teardown is still running. Teardown runs on the thread pool, so ShutdownAsync never blocks and the synchronous Dispose cannot deadlock against a UI-thread continuation. Task.Run captures the execution context, so the connection's read-loop AsyncLocal marker still flows and disposing from a notification handler does not self-wait (covered by a test asserting < 4s against the connection's 5s read-loop timeout).

Bounded, and actually bounded. Teardown waits ServerShutdownTimeout, then cancels the callback's token, then a fixed 5s grace, then abandons and logs. CancellationTokenSource.Cancel() runs registrations synchronously, so the cancellation is started separately — otherwise a blocking caller registration would prevent the grace from ever starting and make the "bounded" wait unbounded. A failed launch skips the graceful wait entirely: nothing is connected, so there is no transport closure for the callback to observe.

Exception preservation. A callback that throws, is canceled, or returns before dialing back surfaces as MtpServerConnectionClosedException with the original as InnerException, instead of a misleading connection timeout. Every teardown helper is non-throwing, and the shared teardown task is wrapped so it can never fault — a faulted shared task would throw from every later disposal.

LaunchAsync(path) is unchanged. Verified against main: identical argument string, failure messages, stderr capture, exit fast-fail and teardown order. One earlier revision of this branch added a grace period to the accept race; review showed it only traded a precise failure for a vague one, so it was removed and the external path is now provably unchanged.

Browser/WASM. Both paths are loopback TCP. LaunchInProcessAsync fails fast with PlatformNotSupportedException there; this does not enable WASM hosting, and the docs say so.

Tests

68 unit tests (net8.0) / 63 (net462, Jsonite path). New coverage: argument array shape; initialize → discover → run → exit; callback faulting synchronously and asynchronously; callback exiting with a code; null task; pre-canceled launch not invoking the callback; cancellation during connect canceling the callback token; connection timeout bounded by the grace rather than ServerShutdownTimeout; ShutdownAsync; ServerExitCode; disposal awaiting the callback; Dispose racing an in-flight ShutdownAsync; Dispose from a notification handler; idempotent disposal; callback faulting during shutdown; unresponsive callback abandoned within the bound; $/cancelRequest; stateful on/off; multi-request single connection; EnvironmentVariables ignored and warned. Stressed 6×/5× consecutively for flakiness.
New acceptance test where a single generated process is simultaneously the embedded host (compiling the packed source-only package) and a real MTP TestApplication with real MSTest over a real [TestClass] — discovering and running its own test over JSON-RPC with no Process.Start anywhere. It also asserts the server's reported process id equals its own.
Existing external-process acceptance test extended to cover ShutdownAsync + ServerExitCode; hostile-consumer compile oracle extended to bind the new API on net462/netstandard2.0/net5.0–net8.0.
Full regression: 13 platform source-package/consumer acceptance, 4 MSTest acceptance, 13 platform ServerTests. build.cmd -pack clean, 0 warnings.

Review history

Four independent review rounds (MTP/MSTest expert reviewer, a design reviewer, and two correctness reviewers) produced 3 major, 10 moderate and 10 minor findings, all addressed. Several were real bugs the tests then locked in — notably Process.ExitCode being unreadable after Process.Dispose(), a TcpListener socket leak when Start() failed, and a teardown path that could throw from a contract documented never to throw.

Open design questions

No IAsyncDisposable. netstandard2.0/net462 would need Microsoft.Bcl.AsyncInterfaces, breaking the package's dependency-free promise. ShutdownAsync() is the substitute. Happy to revisit if the dependency is acceptable.
MtpServerClientOptions is mode-mixed.EnvironmentVariables is external-process only; ServerShutdownTimeout is in-process only. Nesting per-transport options would age better, but EnvironmentVariables already shipped at the top level, so it cannot be done non-breakingly now.
The 5s cancellation grace is a fixed constant, not an option.
The two paths still build different argument shapes — deliberate, to keep the shipped external command line byte-identical. Commented at the call site.
No ConnectAsync(TcpClient/Stream, options) factory — the existing MtpServerClient(MtpJsonRpcConnection, options) constructor already covers "wrap an existing transport", and the guidance is to minimize injected surface. Easy to add if wanted.

Embedded hosts such as MAUI or Android/iOS test apps cannot spawn a child process, so `MtpServerClient.LaunchAsync(path)` was unusable for them and they had to reimplement the listener, the server-mode arguments, the connect race, the serializer/formatter ordering, the transport and the shutdown coordination by hand -- reintroducing the bugs the canonical client already solves. Add `MtpServerClient.LaunchInProcessAsync(callback, options, cancellationToken)`. The client keeps ownership of everything except "how to run the application": it binds the loopback listener, generates the complete server-mode argument array, races the connect against callback failure/completion, caller cancellation and the connection timeout, and owns a bounded shutdown that closes the transport, then cancels the callback token, then abandons it rather than hanging the caller. A callback failure before connection is surfaced as `MtpServerConnectionClosedException` with the caller's exception preserved as the inner exception, and teardown failures are only logged so they can never replace the primary failure. The shared transport setup is extracted from `MtpServerProcess` into `MtpServerConnector` and both launch paths now flow through `IMtpServerHost`, so the external-process behavior is unchanged while the two paths cannot drift. The one behavior change is an improvement shared by both: when the server is seen to have stopped, a still-pending accept gets a bounded final grace so a connection established just before the stop is not discarded in favor of a misleading "stopped before connecting back" failure. The path is loopback TCP, so it fails fast with `PlatformNotSupportedException` on browser/WASM; it does not enable WASM hosting. Fixes#10890 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Two independent reviews of the in-process launch path raised the same top finding: `Dispose()` waits for the hosted application synchronously on the calling thread, and on the very platforms this feature targets (MAUI, Android, iOS) a multi-second block on the UI thread trips the ANR / watchdog. Add `IMtpServerClient.ShutdownAsync()` (and `IMtpServerHost.ShutdownAsync`) so the same teardown can be awaited instead. `IAsyncDisposable` stays rejected -- netstandard2.0 and net462 would need `Microsoft.Bcl.AsyncInterfaces`, which breaks the package's dependency-free promise -- but rejecting the interface is not a reason to have no asynchronous shutdown at all. `Dispose()` remains, is still idempotent, and returns immediately after `ShutdownAsync`. Remove the accept grace period. The previous commit let a still-pending accept win for ~100ms after the server was seen to have stopped, on the theory that a connection established just before the stop should not be discarded. That theory does not hold: if the server has stopped, the socket belongs to a dead peer, so the grace only traded a precise failure (exit code plus captured stderr) for a generic connection-closed error on the first request, and its uncancellable delay could let the server failure beat a concurrent caller cancellation. Dropping it also makes the external-process path provably unchanged, so the changelog no longer needs a `Changed` entry. Other review fixes: * Expose `IMtpServerClient.ServerExitCode`. The callback signature already demanded a `Task<int>`, but after a successful session the value was unreachable, so an embedded host whose `Main` must return it had to capture it in a closure. * Do not dispose the server's `CancellationTokenSource` when the callback was abandoned while still running: it holds the token, and `token.WaitHandle` or `CreateLinkedTokenSource` would then throw `ObjectDisposedException` inside the caller's own code. * Skip the graceful wait entirely on a failed launch. Nothing is connected, so there is no transport closure for the callback to observe; only the fixed cancellation grace applies and an unwinding caller no longer pays `ServerShutdownTimeout`. * Clamp bounded waits instead of trusting the caller's `TimeSpan`. A negative value (or `Timeout.InfiniteTimeSpan`) made `Task.Delay` throw from a path documented never to throw; an oversized one is capped to the largest delay .NET Framework accepts. * Move `TcpListener.Start()` inside the cleanup `try`: it creates the socket before binding, so a bind failure leaked it because the caller never received a listener it could stop. * Log a late failure from an abandoned callback rather than only observing it. * Cache the in-process host's `ProcessId` instead of allocating a `Process` per property read. * Document that `Dispose()` blocks, that cancellation is bounded rather than immediate, and why the two launch paths still build different argument shapes; fix the shadowed `cancellationToken` in the PACKAGE.md sample. Tests: cover `ShutdownAsync`, `ServerExitCode`, and fail-fast on a server that stops without connecting; make `Dispose_IsIdempotent` able to fail by asserting on observed transport closes rather than callback invocations; replace the hand-rolled throws helper with `Assert.ThrowsExactlyAsync`; tighten the shutdown-bound assertion; and stop leaking `Process` handles. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

A third review round found three ways the teardown contract did not hold up under concurrency. `Dispose()` and `ShutdownAsync()` each had their own `Interlocked.Exchange` guard, so whichever call lost the race returned while teardown was still running -- reporting to its caller that the application had stopped when it had not. Replace both guards with one lazily started, shared teardown task: every entry point now joins the same task, so `Dispose` after (or racing) `ShutdownAsync` returns only once the application has actually stopped, and the whole thing stays idempotent. `MtpServerClient` no longer keeps its own guard either; it delegates to the host, whose teardown is the single source of truth. `ShutdownAsync()` could still block the very thread it exists to protect: it closed the transport before its first await, and closing the connection waits up to five seconds for the read loop. The shared task is started with `Task.Run`, so the whole teardown -- transport close included -- runs on the thread pool and `ShutdownAsync` returns immediately. `MtpServerProcess` likewise now runs its (bounded but synchronous) kill off the calling thread rather than pretending to be async while blocking. `CancellationTokenSource.Cancel()` executes registrations synchronously on the calling thread, so a caller registration that blocked would prevent the five-second cancellation grace from ever starting and make the "bounded" shutdown unbounded. Start the cancellation separately and begin the grace regardless. When a registration is still executing once the callback has finished, the source is reported as unsafe to dispose for the same reason an abandoned callback is: leaking one `CancellationTokenSource` beats a use-after-dispose inside the caller's code. Adds a test that a `Dispose` racing an in-flight `ShutdownAsync` blocks until the shared teardown completes, and that both share one teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

… fix ExitCode A fourth review round found that only the in-process host had been converted to the shared-teardown design. `MtpServerProcess` kept its old early-return guard, so on the external-process path a `Dispose()` that followed or raced `ShutdownAsync()` returned while `SafeKill`'s bounded `WaitForExit` was still running on another thread -- losing exactly the guarantee that wait exists for (a caller may delete the application directory immediately after disposal), and making `await ShutdownAsync()` report that the server had stopped when it had not. That contradicted the comment the previous commit added to `MtpServerClient.Dispose`, which claimed both paths joined an in-flight teardown. Convert `MtpServerProcess` to the same lazily created shared task, so the contract is now uniform across both implementations of `IMtpServerHost`, and reconcile the interface docs, which previously described the opposite rule. Adding real coverage for `ShutdownAsync` on the external-process path then surfaced a genuine bug: `MtpServerProcess.ExitCode` was always `null` after teardown, because a `Process` cannot be queried once disposed. Capture the exit code during teardown instead -- before the kill, so an application that already exited on its own reports its real code rather than the kill's, and before `Process.Dispose()`, after which nothing is readable. Also fixes a race in the in-process test fixture that made the suite flaky on net462: the callback published its `FakeMtpServer` only after its own connect call returned, but the client's accept can complete first, so a test could reach `Value` before the callback had set it. The fixture now exposes a `Connected` signal the launch helper awaits. 6/6 clean net462 runs and 5/5 clean net8.0 runs afterwards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

+ if (serverTask is not null)
+ {
+ // The launch is being abandoned, so skip the graceful wait entirely: there is no connected
+ // transport whose closure could signal the callback, and the caller (often a canceling one) is
+ // waiting on this unwind. A zero graceful timeout goes straight to cancel-then-grace.
+ if (!await ShutdownServerAsync(serverTask, serverCancellation!, TimeSpan.Zero, logger).ConfigureAwait(false))
+ {
+ // The callback is still running and still holds the token; disposing its source now would
+ // turn a clean abandonment into an ObjectDisposedException inside the caller's own code.
+ throw;
+ }
+ }


github-actions · 2026-08-31T19:32:23Z

🧪 Expert test review — PR #10898

Grade	Test	Mutation	Notes	How to improve
B (80–89)	new `MtpServerClientInProcessTests. Dispose_CallbackFaultsDuringShutdown_ DoesNotThrow`	2/3 killed	Reads `server.Completion.Exception` but never asserts on it, so a mutation changing the faulted exception's identity/message survives.	Assert the captured exception's message/type instead of discarding it after touching `.Exception`.
B (80–89)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_HonorsTheStatefulOption`	3/4 killed	Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on `client.Capabilities` is not caught.	Also assert `client.Capabilities.IsStateful` reflects the negotiated round-trip value, not only the sent request.
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_PassesCompleteServerModeArguments`	5/5 killed	Verifies exact ordered arguments, dynamic port and fixed count; strong regression guard for the generated argument array.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_DrivesInitializeDiscoverRunAndExit`	6/6 killed	End-to-end drive through initialize/discover/run/exit with distinct assertions per state transition.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsBeforeConnecting_ PreservesCallbackException`	2/2 killed	Asserts the exact exception instance is preserved as inner exception, not just its type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsAsynchronouslyBeforeConnecting_ PreservesCallbackException`	2/2 killed	Covers the async-throw variant with the same identity assertion as the sync case.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsWithoutConnecting_ FailsFastInsteadOfWaitingOutTheTimeout`	3/3 killed	Checks both the reported exit code in the message and a timing bound guarding against a slow-timeout regression.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsBeforeConnecting_ ReportsExitCode`	2/2 killed	Asserts the specific exit code surfaces in the exception message.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackReturnsNullTask_Fails`	2/2 killed	Asserts the specific inner exception type for a misbehaving callback returning a null task.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_NullCallback_Throws`	1/1 killed	Focused single-assertion guard-clause test using the exact exception type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_AlreadyCanceled_ DoesNotInvokeCallback`	2/2 killed	Verifies both the cancellation exception and, via an interlocked counter, that the callback never ran.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CanceledWhileConnecting_ CancelsTheCallbackToken`	2/2 killed	Confirms both the caller-side cancellation and that the callback's own token observed cancellation.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_ConnectionTimeoutElapses_ FailsWithTimeoutMessage`	3/3 killed	Distinguishes the connection timeout from the deliberately huge shutdown timeout via message and elapsed-time bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_ClosesTransportAndAwaitsTheCallback`	4/4 killed	Checks pre/post completion state, the callback's returned exit code, and the client's reported exit code.	—
A (90–100)	new `MtpServerClientInProcessTests. ShutdownAsync_ClosesTransportAndAwaitsTheCallback_ WithoutBlocking`	4/4 killed	Verifies await-completion, exit code, a fast follow-up Dispose, and single-teardown count together.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_FromANotificationHandler_ DoesNotSelfWaitOnTheReadLoop`	1/1 killed	Targeted timing assertion guards against a specific re-entrancy deadlock regression.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_WhileShutdownAsyncIsInFlight_ WaitsForTheSameTeardown`	3/3 killed	Uses a controlled release gate to prove the racing Dispose genuinely blocks, then confirms a single shared teardown.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_IsIdempotent`	3/3 killed	Confirms exactly one transport close and one completion despite three Dispose calls, plus a fast-return bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_CallbackIgnoresShutdown_ ReturnsWithinTheDocumentedBound`	2/2 killed	Verifies both the documented abandonment time bound and the logged "abandoning" diagnostic.	—
A (90–100)	new `MtpServerClientInProcessTests. RunTestsAsync_Canceled_ SendsCancelRequestToTheHostedApplication`	2/2 killed	Confirms both the client-side cancellation exception and the wire-level cancel notification reaching the fake server.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_MultipleRequestsOnOneSession_ ReuseTheSameConnection`	3/3 killed	Checks keep-alive negotiation, single-connection reuse count, and total request count together.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_IgnoresEnvironmentVariablesAndWarns`	1/1 killed	Asserts the specific warning content naming the ignored option.	—
A (90–100)	new `MtpServerClientInProcessAcceptanceTests. InProcessHost_ DiscoversAndRunsItsOwnMSTestNodes_ WithoutStartingAProcess`	5/5 killed	Real end-to-end embedded-host run asserting build success, exit code, and each marker line the generated app emits.	—
A (90–100)	mod `MtpServerClientAcceptanceTests. DiscoverAndRun_ViaSourcePackageClient_ ReportsExpectedTestNode`	1/1 killed	New lines assert the external-process ShutdownAsync/Dispose share a teardown and report an exit code.	—
A (90–100)	mod `MtpServerClientSourcePackageConsumerTests. HostileConsumer_ CompilesAgainstPackedSource`	N/A	Compile-only guard; added lines correctly extend surface coverage to `ShutdownAsync` and the in-process launch path.	—

_{This advisory comment was generated automatically. Grades are heuristic

and informational — they do not block merging. Suggestions on the Files

changed tab can be applied with one click. Re-run with
/review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · [◷]( · ◷)

github-actions · 2026-08-31T19:32:24Z

🧵 Parallel-safety audit — PR #10898

Parallelization — audited assemblies:

Test assembly	Scope	Workers	Analyzer coverage
`MSTest.Acceptance.IntegrationTests`	`MethodLevel` (default `[assembly: Parallelize(Scope = ExecutionScope.MethodLevel, Workers = 0)]` in `Program.cs`)	CPU count	coverable once the parallel-safety analyzers ship (attribute-based opt-in)
`Microsoft.Testing.Platform.Acceptance.IntegrationTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped
`Microsoft.Testing.Platform.ServerMode.Client.Sources.UnitTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped

This PR did not touch any .runsettings/testconfig.json/Directory.Build.* parallelization config — no assembly's scope changed.

Findings: A (global-state) 0 · B (paths) 0 · C (declaration) 0 · D (over-serialization) 0 — by severity: Critical 0 · High 0 · Warning 0 · Info 0.

No unsafe call sites found in the lines this PR added or modified.

MtpServerClientAcceptanceTests.cs — added lines only call client.ShutdownAsync() / assert ServerExitCode; no process-global state touched.
MtpServerClientSourcePackageConsumerTests.cs — added lines call into an isolated generated test asset (DriveAsync/DriveInProcessAsync); no shared/relative paths, env vars, culture, or console state mutated at the assembly level (Console.WriteLine here writes to the generated app's own redirected output, not the auditing process).
MtpServerClientInProcessAcceptanceTests.cs (new) — uses TestAsset.GenerateAssetAsync, which allocates a unique GUID-suffixed TempDirectory per instance (TestAsset.cs:19-26, throws if a path already exists) — no shared-path collision. CreateChildEnvironment() builds a fresh Dictionary passed only to the spawned child process's environment, not Environment.SetEnvironmentVariable, so it does not mutate this process's state.
FakeMtpServer.cs / MtpServerClientInProcessTests.cs (new) — each test constructs its own TcpListener/FakeMtpServer bound to an OS-assigned loopback port (new TcpListener(IPAddress.Loopback, 0)), so there is no fixed-port collision between concurrently-running tests; no static/process-global state is written.

Nothing here implies a testability or smell concern beyond this audit's scope.

_{Advisory only — heuristic, non-blocking. Re-run with /parallel-audit. This audit answers "is it parallel-safe?"; for testability, smells, or flakiness see the detect-static-dependencies / test-smell-detection / test-anti-patterns analyses.}

🤖 Automated content by GitHub Copilot. Generated by the Parallel-safety audit on PR (on open / sync) workflow. · auto · 117.1 AIC · ⌖ 3.53 AIC · ⊞ 24.8K · [◷]( · ◷)

github-actions

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · ◷

github-actions · 2026-08-31T19:32:25Z

+ client.Dispose();
+
+ Assert.IsTrue(server.Completion.IsFaulted);
+ _ = server.Completion.Exception;


🧪 Test review · Grade B (80–89) — Reads server.Completion.Exception but never asserts on it, so a mutation changing the faulted exception's identity/message would survive.
Assert the captured exception's message/type instead of discarding it after touching .Exception.
Suggested change
_=server.Completion.Exception;
Exception?faultException=server.Completion.Exception?.GetBaseException();
Assert.IsInstanceOfType<InvalidOperationException>(faultException);
Assert.AreEqual("The application failed while shutting down.",faultException!.Message);

github-actions · 2026-08-31T19:32:25Z

+ ? typed
+ : SerializerUtilities.Deserialize<InitializeRequestArgs>((IDictionary<string, object?>)initialize.Params!);
+
+ Assert.AreEqual(isStateful, args.Capabilities.IsStateful, "The in-process path must forward the client's stateful capability unchanged.");


🧪 Test review · Grade B (80–89) — Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on client.Capabilities is not caught.
Also assert client.Capabilities.IsStateful reflects the negotiated round-trip value, not only the sent request.
Suggested change
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,capabilities.IsStateful,"The negotiated capability the client stores must reflect the server's round trip, not just the sent request.");

github-actions

Note

🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

✅ 22/22 dimensions clean — no findings.

Summary: This PR adds MtpServerClient.LaunchInProcessAsync for embedded hosts (MAUI, Android/iOS) that cannot Process.Start. The implementation is thorough and well-structured:

Threading & Concurrency: Shutdown is idempotent via lock + lazy Task, Volatile.Read/Write for cross-thread exit code, Task.Run avoids sync-context deadlocks. No shared mutable state without synchronization.
Security & IPC: Loopback TCP only, no command-line injection surface (arguments are array-based), WASM fails fast with PlatformNotSupportedException.
Public API: All new API is internal (source-only package). No init accessors. No PublicAPI.Unshipped.txt changes needed.
Performance: GetCurrentProcessId() snapshotted once in constructor. No hot-path allocations. WaitBoundedAsync clamps timeouts correctly.
Cross-TFM: #if NET8_0_OR_GREATER guard on AcceptTcpClientAsync(CancellationToken). RuntimeInformation used for OS detection on net462.
Resource Management: Every disposable (TcpListener, TcpClient, CancellationTokenSource, Process) has cleanup in both success and error paths. Pending accepts are neutralized. ServerCancellation is only disposed when safe.
Defensive Coding: Callback exceptions are wrapped as InnerException in MtpServerConnectionClosedException. Null task from callback is caught. Cancel() runs on thread pool to avoid blocking registration.
Error Handling: Shared teardown task wrapped in catch-all so it never faults. ObserveFailure prevents UnobservedTaskException. Every teardown helper is non-throwing.
Tests: 68 unit tests + acceptance tests covering argument shape, lifecycle, faults, cancellation, timeouts, idempotent disposal, notification-handler disposal, and the real end-to-end in-process path.
Documentation: Changelog, protocol intro, PACKAGE.md all updated. XML doc comments are thorough.

The refactoring of shared transport logic into MtpServerConnector and the IMtpServerHost abstraction is clean and prevents the two launch paths from drifting. The existing LaunchAsync(path) path is provably unchanged (same argument string, same failure messages, same teardown order).

Copilot

Copilot review overview

Review tier: Balanced
Findings: 3

New issues introduced by this change (3)

Severity	Finding
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `MtpJsonRpcConnection.Dispose()` closes the socket and then waits up to its 5-second read-loop…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `ShutdownAsync` writes this nullable property from the teardown worker while callers can read…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — A canceled callback takes this branch without an inner exception, although the package…

What changed in this PR

Adds in-process launch support to the source-only MTP server-mode client for embedded/mobile hosts, addressing #10890 and enabling scenarios related to #9809.

Changes:

Adds callback-based in-process hosting with shared transport setup.
Adds bounded asynchronous shutdown and server exit-code reporting.
Adds documentation, unit coverage, and real-MSTest acceptance coverage.

File	Description
`MtpServerClientInProcessTests.cs`	Tests in-process lifecycle and protocol behavior.
`FakeMtpServer.cs`	Supports server-to-client dial-back mode.
`MtpServerClientInProcessAcceptanceTests.cs`	Exercises a real in-process MSTest application.
`MtpServerClientAcceptanceTests.cs`	Covers external-process shutdown and exit code.
`MtpServerClientSourcePackageConsumerTests.cs`	Extends source-package compile coverage.
`PACKAGE.md`	Documents embedded-host usage and lifecycle.
`MtpServerProcess.cs`	Adopts shared host and shutdown abstractions.
`MtpServerInProcessHost.cs`	Implements callback hosting and teardown.
`MtpServerConnector.cs`	Centralizes listener and transport setup.
`MtpServerClientOptions.cs`	Adds the shutdown timeout option.
`MtpServerClientExceptions.cs`	Supports preserved inner exceptions.
`MtpServerClient.cs`	Exposes in-process launch and shutdown.
`IMtpServerHost.cs`	Defines common host ownership behavior.
`IMtpServerClient.cs`	Adds shutdown and exit-code members.
`001-protocol-intro.md`	Documents reference-client launch modes.
`Changelog-Platform.md`	Records the new embedded-host capability.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+ Connection.Dispose();
+ SafeDispose(_client, _logger, "Disposing the accepted client socket");
+ MtpServerConnector.SafeStop(_listener, _logger);
+
+ bool stopped = await ShutdownServerAsync(_serverTask, _serverCancellation, _shutdownTimeout, _logger).ConfigureAwait(false);


+ /// Gets the exit code the hosted application returned, or <see langword="null"/> while it is still
+ /// running (or when it failed or was abandoned rather than returning one).
+ /// </summary>
+ public int? ExitCode { get; private set; }


+ if (serverTask.IsCanceled)
+ {
+ return new MtpServerConnectionClosedException(
+ "The in-process Microsoft.Testing.Platform application was canceled before connecting back.");
+ }


Amaury Levé (Evangelink) · 2026-08-31T19:22:25Z

Fixes#10890

Why

MtpServerClient.LaunchAsync(path) owns the loopback listener and starts the test application with Process.Start. That is right for IDE and desktop tooling, but unusable for embedded hosts — MAUI apps, Android/iOS test apps — where the MTP application already runs in the caller's process.

Today those hosts have to reimplement the listener, the --server/host/port arguments, the race between connection and startup failure, the serializer-before-formatter registration order, TcpMessageHandler + MtpJsonRpcConnection construction, and the exit / transport-close / server-completion shutdown dance. That defeats the point of shipping a canonical client and makes it easy to reintroduce bugs this package already solved: partial frame writes, ignored JSON-RPC errors, missing $/cancelRequest, unbounded waits, exception masking.

A concrete consumer is the DeviceRunners MSTest visual-runner work (mattleibow/DeviceRunners#157, #9809).

What

usingIMtpServerClientclient=awaitMtpServerClient.LaunchInProcessAsync(async(serverArgs,token)=>{ITestApplicationBuilderbuilder=awaitTestApplication.CreateBuilderAsync(serverArgs);builder.AddMSTest(()=>testAssemblies);usingITestApplicationapp=awaitbuilder.BuildAsync();returnawaitapp.RunAsync();},options,cancellationToken);awaitclient.InitializeAsync();awaitclient.DiscoverTestsAsync();awaitclient.RunTestsAsync();awaitclient.ExitAsync();awaitclient.ShutdownAsync();

The caller supplies only "how to run the application". The client keeps everything else: it binds the listener, generates the complete argument array (--server jsonrpc --client-host 127.0.0.1 --client-port <port> --no-banner), races the connect, builds the transport, and owns a bounded shutdown.

New API on the injected (still internal) surface:

Member	Purpose
`MtpServerClient.LaunchInProcessAsync(callback, options, ct)`	The embedded-host launch path. Async only — blocking the launching thread can deadlock the application being launched.
`IMtpServerClient.ShutdownAsync()`	Non-blocking teardown.
`IMtpServerClient.ServerExitCode`	The value the application returned.
`MtpServerClientOptions.ServerShutdownTimeout`	Graceful shutdown bound (default 30s).

Internally, the transport setup was extracted from MtpServerProcess into MtpServerConnector, and both launch paths now sit behind IMtpServerHost, so they cannot drift.

Behavior worth reviewing

Ownership and shutdown. Both Dispose() and ShutdownAsync() join one lazily created shared teardown task, on both hosts. A Dispose that follows or races ShutdownAsync therefore returns only once the server has actually stopped, rather than reporting success while teardown is still running. Teardown runs on the thread pool, so ShutdownAsync never blocks and the synchronous Dispose cannot deadlock against a UI-thread continuation. Task.Run captures the execution context, so the connection's read-loop AsyncLocal marker still flows and disposing from a notification handler does not self-wait (covered by a test asserting < 4s against the connection's 5s read-loop timeout).

Bounded, and actually bounded. Teardown waits ServerShutdownTimeout, then cancels the callback's token, then a fixed 5s grace, then abandons and logs. CancellationTokenSource.Cancel() runs registrations synchronously, so the cancellation is started separately — otherwise a blocking caller registration would prevent the grace from ever starting and make the "bounded" wait unbounded. A failed launch skips the graceful wait entirely: nothing is connected, so there is no transport closure for the callback to observe.

Exception preservation. A callback that throws, is canceled, or returns before dialing back surfaces as MtpServerConnectionClosedException with the original as InnerException, instead of a misleading connection timeout. Every teardown helper is non-throwing, and the shared teardown task is wrapped so it can never fault — a faulted shared task would throw from every later disposal.

LaunchAsync(path) is unchanged. Verified against main: identical argument string, failure messages, stderr capture, exit fast-fail and teardown order. One earlier revision of this branch added a grace period to the accept race; review showed it only traded a precise failure for a vague one, so it was removed and the external path is now provably unchanged.

Browser/WASM. Both paths are loopback TCP. LaunchInProcessAsync fails fast with PlatformNotSupportedException there; this does not enable WASM hosting, and the docs say so.

Tests

68 unit tests (net8.0) / 63 (net462, Jsonite path). New coverage: argument array shape; initialize → discover → run → exit; callback faulting synchronously and asynchronously; callback exiting with a code; null task; pre-canceled launch not invoking the callback; cancellation during connect canceling the callback token; connection timeout bounded by the grace rather than ServerShutdownTimeout; ShutdownAsync; ServerExitCode; disposal awaiting the callback; Dispose racing an in-flight ShutdownAsync; Dispose from a notification handler; idempotent disposal; callback faulting during shutdown; unresponsive callback abandoned within the bound; $/cancelRequest; stateful on/off; multi-request single connection; EnvironmentVariables ignored and warned. Stressed 6×/5× consecutively for flakiness.
New acceptance test where a single generated process is simultaneously the embedded host (compiling the packed source-only package) and a real MTP TestApplication with real MSTest over a real [TestClass] — discovering and running its own test over JSON-RPC with no Process.Start anywhere. It also asserts the server's reported process id equals its own.
Existing external-process acceptance test extended to cover ShutdownAsync + ServerExitCode; hostile-consumer compile oracle extended to bind the new API on net462/netstandard2.0/net5.0–net8.0.
Full regression: 13 platform source-package/consumer acceptance, 4 MSTest acceptance, 13 platform ServerTests. build.cmd -pack clean, 0 warnings.

Review history

Four independent review rounds (MTP/MSTest expert reviewer, a design reviewer, and two correctness reviewers) produced 3 major, 10 moderate and 10 minor findings, all addressed. Several were real bugs the tests then locked in — notably Process.ExitCode being unreadable after Process.Dispose(), a TcpListener socket leak when Start() failed, and a teardown path that could throw from a contract documented never to throw.

Open design questions

No IAsyncDisposable. netstandard2.0/net462 would need Microsoft.Bcl.AsyncInterfaces, breaking the package's dependency-free promise. ShutdownAsync() is the substitute. Happy to revisit if the dependency is acceptable.
MtpServerClientOptions is mode-mixed.EnvironmentVariables is external-process only; ServerShutdownTimeout is in-process only. Nesting per-transport options would age better, but EnvironmentVariables already shipped at the top level, so it cannot be done non-breakingly now.
The 5s cancellation grace is a fixed constant, not an option.
The two paths still build different argument shapes — deliberate, to keep the shipped external command line byte-identical. Commented at the call site.
No ConnectAsync(TcpClient/Stream, options) factory — the existing MtpServerClient(MtpJsonRpcConnection, options) constructor already covers "wrap an existing transport", and the guidance is to minimize injected surface. Easy to add if wanted.

Embedded hosts such as MAUI or Android/iOS test apps cannot spawn a child process, so `MtpServerClient.LaunchAsync(path)` was unusable for them and they had to reimplement the listener, the server-mode arguments, the connect race, the serializer/formatter ordering, the transport and the shutdown coordination by hand -- reintroducing the bugs the canonical client already solves. Add `MtpServerClient.LaunchInProcessAsync(callback, options, cancellationToken)`. The client keeps ownership of everything except "how to run the application": it binds the loopback listener, generates the complete server-mode argument array, races the connect against callback failure/completion, caller cancellation and the connection timeout, and owns a bounded shutdown that closes the transport, then cancels the callback token, then abandons it rather than hanging the caller. A callback failure before connection is surfaced as `MtpServerConnectionClosedException` with the caller's exception preserved as the inner exception, and teardown failures are only logged so they can never replace the primary failure. The shared transport setup is extracted from `MtpServerProcess` into `MtpServerConnector` and both launch paths now flow through `IMtpServerHost`, so the external-process behavior is unchanged while the two paths cannot drift. The one behavior change is an improvement shared by both: when the server is seen to have stopped, a still-pending accept gets a bounded final grace so a connection established just before the stop is not discarded in favor of a misleading "stopped before connecting back" failure. The path is loopback TCP, so it fails fast with `PlatformNotSupportedException` on browser/WASM; it does not enable WASM hosting. Fixes#10890 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Two independent reviews of the in-process launch path raised the same top finding: `Dispose()` waits for the hosted application synchronously on the calling thread, and on the very platforms this feature targets (MAUI, Android, iOS) a multi-second block on the UI thread trips the ANR / watchdog. Add `IMtpServerClient.ShutdownAsync()` (and `IMtpServerHost.ShutdownAsync`) so the same teardown can be awaited instead. `IAsyncDisposable` stays rejected -- netstandard2.0 and net462 would need `Microsoft.Bcl.AsyncInterfaces`, which breaks the package's dependency-free promise -- but rejecting the interface is not a reason to have no asynchronous shutdown at all. `Dispose()` remains, is still idempotent, and returns immediately after `ShutdownAsync`. Remove the accept grace period. The previous commit let a still-pending accept win for ~100ms after the server was seen to have stopped, on the theory that a connection established just before the stop should not be discarded. That theory does not hold: if the server has stopped, the socket belongs to a dead peer, so the grace only traded a precise failure (exit code plus captured stderr) for a generic connection-closed error on the first request, and its uncancellable delay could let the server failure beat a concurrent caller cancellation. Dropping it also makes the external-process path provably unchanged, so the changelog no longer needs a `Changed` entry. Other review fixes: * Expose `IMtpServerClient.ServerExitCode`. The callback signature already demanded a `Task<int>`, but after a successful session the value was unreachable, so an embedded host whose `Main` must return it had to capture it in a closure. * Do not dispose the server's `CancellationTokenSource` when the callback was abandoned while still running: it holds the token, and `token.WaitHandle` or `CreateLinkedTokenSource` would then throw `ObjectDisposedException` inside the caller's own code. * Skip the graceful wait entirely on a failed launch. Nothing is connected, so there is no transport closure for the callback to observe; only the fixed cancellation grace applies and an unwinding caller no longer pays `ServerShutdownTimeout`. * Clamp bounded waits instead of trusting the caller's `TimeSpan`. A negative value (or `Timeout.InfiniteTimeSpan`) made `Task.Delay` throw from a path documented never to throw; an oversized one is capped to the largest delay .NET Framework accepts. * Move `TcpListener.Start()` inside the cleanup `try`: it creates the socket before binding, so a bind failure leaked it because the caller never received a listener it could stop. * Log a late failure from an abandoned callback rather than only observing it. * Cache the in-process host's `ProcessId` instead of allocating a `Process` per property read. * Document that `Dispose()` blocks, that cancellation is bounded rather than immediate, and why the two launch paths still build different argument shapes; fix the shadowed `cancellationToken` in the PACKAGE.md sample. Tests: cover `ShutdownAsync`, `ServerExitCode`, and fail-fast on a server that stops without connecting; make `Dispose_IsIdempotent` able to fail by asserting on observed transport closes rather than callback invocations; replace the hand-rolled throws helper with `Assert.ThrowsExactlyAsync`; tighten the shutdown-bound assertion; and stop leaking `Process` handles. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

A third review round found three ways the teardown contract did not hold up under concurrency. `Dispose()` and `ShutdownAsync()` each had their own `Interlocked.Exchange` guard, so whichever call lost the race returned while teardown was still running -- reporting to its caller that the application had stopped when it had not. Replace both guards with one lazily started, shared teardown task: every entry point now joins the same task, so `Dispose` after (or racing) `ShutdownAsync` returns only once the application has actually stopped, and the whole thing stays idempotent. `MtpServerClient` no longer keeps its own guard either; it delegates to the host, whose teardown is the single source of truth. `ShutdownAsync()` could still block the very thread it exists to protect: it closed the transport before its first await, and closing the connection waits up to five seconds for the read loop. The shared task is started with `Task.Run`, so the whole teardown -- transport close included -- runs on the thread pool and `ShutdownAsync` returns immediately. `MtpServerProcess` likewise now runs its (bounded but synchronous) kill off the calling thread rather than pretending to be async while blocking. `CancellationTokenSource.Cancel()` executes registrations synchronously on the calling thread, so a caller registration that blocked would prevent the five-second cancellation grace from ever starting and make the "bounded" shutdown unbounded. Start the cancellation separately and begin the grace regardless. When a registration is still executing once the callback has finished, the source is reported as unsafe to dispose for the same reason an abandoned callback is: leaking one `CancellationTokenSource` beats a use-after-dispose inside the caller's code. Adds a test that a `Dispose` racing an in-flight `ShutdownAsync` blocks until the shared teardown completes, and that both share one teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

… fix ExitCode A fourth review round found that only the in-process host had been converted to the shared-teardown design. `MtpServerProcess` kept its old early-return guard, so on the external-process path a `Dispose()` that followed or raced `ShutdownAsync()` returned while `SafeKill`'s bounded `WaitForExit` was still running on another thread -- losing exactly the guarantee that wait exists for (a caller may delete the application directory immediately after disposal), and making `await ShutdownAsync()` report that the server had stopped when it had not. That contradicted the comment the previous commit added to `MtpServerClient.Dispose`, which claimed both paths joined an in-flight teardown. Convert `MtpServerProcess` to the same lazily created shared task, so the contract is now uniform across both implementations of `IMtpServerHost`, and reconcile the interface docs, which previously described the opposite rule. Adding real coverage for `ShutdownAsync` on the external-process path then surfaced a genuine bug: `MtpServerProcess.ExitCode` was always `null` after teardown, because a `Process` cannot be queried once disposed. Capture the exit code during teardown instead -- before the kill, so an application that already exited on its own reports its real code rather than the kill's, and before `Process.Dispose()`, after which nothing is readable. Also fixes a race in the in-process test fixture that made the suite flaky on net462: the callback published its `FakeMtpServer` only after its own connect call returned, but the client's accept can complete first, so a test could reach `Value` before the callback had set it. The fixture now exposes a `Connected` signal the launch helper awaits. 6/6 clean net462 runs and 5/5 clean net8.0 runs afterwards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

+ if (serverTask is not null)
+ {
+ // The launch is being abandoned, so skip the graceful wait entirely: there is no connected
+ // transport whose closure could signal the callback, and the caller (often a canceling one) is
+ // waiting on this unwind. A zero graceful timeout goes straight to cancel-then-grace.
+ if (!await ShutdownServerAsync(serverTask, serverCancellation!, TimeSpan.Zero, logger).ConfigureAwait(false))
+ {
+ // The callback is still running and still holds the token; disposing its source now would
+ // turn a clean abandonment into an ObjectDisposedException inside the caller's own code.
+ throw;
+ }
+ }


github-actions · 2026-08-31T19:32:23Z

🧪 Expert test review — PR #10898

Grade	Test	Mutation	Notes	How to improve
B (80–89)	new `MtpServerClientInProcessTests. Dispose_CallbackFaultsDuringShutdown_ DoesNotThrow`	2/3 killed	Reads `server.Completion.Exception` but never asserts on it, so a mutation changing the faulted exception's identity/message survives.	Assert the captured exception's message/type instead of discarding it after touching `.Exception`.
B (80–89)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_HonorsTheStatefulOption`	3/4 killed	Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on `client.Capabilities` is not caught.	Also assert `client.Capabilities.IsStateful` reflects the negotiated round-trip value, not only the sent request.
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_PassesCompleteServerModeArguments`	5/5 killed	Verifies exact ordered arguments, dynamic port and fixed count; strong regression guard for the generated argument array.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_DrivesInitializeDiscoverRunAndExit`	6/6 killed	End-to-end drive through initialize/discover/run/exit with distinct assertions per state transition.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsBeforeConnecting_ PreservesCallbackException`	2/2 killed	Asserts the exact exception instance is preserved as inner exception, not just its type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackFaultsAsynchronouslyBeforeConnecting_ PreservesCallbackException`	2/2 killed	Covers the async-throw variant with the same identity assertion as the sync case.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsWithoutConnecting_ FailsFastInsteadOfWaitingOutTheTimeout`	3/3 killed	Checks both the reported exit code in the message and a timing bound guarding against a slow-timeout regression.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackExitsBeforeConnecting_ ReportsExitCode`	2/2 killed	Asserts the specific exit code surfaces in the exception message.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CallbackReturnsNullTask_Fails`	2/2 killed	Asserts the specific inner exception type for a misbehaving callback returning a null task.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_NullCallback_Throws`	1/1 killed	Focused single-assertion guard-clause test using the exact exception type.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_AlreadyCanceled_ DoesNotInvokeCallback`	2/2 killed	Verifies both the cancellation exception and, via an interlocked counter, that the callback never ran.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_CanceledWhileConnecting_ CancelsTheCallbackToken`	2/2 killed	Confirms both the caller-side cancellation and that the callback's own token observed cancellation.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_ConnectionTimeoutElapses_ FailsWithTimeoutMessage`	3/3 killed	Distinguishes the connection timeout from the deliberately huge shutdown timeout via message and elapsed-time bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_ClosesTransportAndAwaitsTheCallback`	4/4 killed	Checks pre/post completion state, the callback's returned exit code, and the client's reported exit code.	—
A (90–100)	new `MtpServerClientInProcessTests. ShutdownAsync_ClosesTransportAndAwaitsTheCallback_ WithoutBlocking`	4/4 killed	Verifies await-completion, exit code, a fast follow-up Dispose, and single-teardown count together.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_FromANotificationHandler_ DoesNotSelfWaitOnTheReadLoop`	1/1 killed	Targeted timing assertion guards against a specific re-entrancy deadlock regression.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_WhileShutdownAsyncIsInFlight_ WaitsForTheSameTeardown`	3/3 killed	Uses a controlled release gate to prove the racing Dispose genuinely blocks, then confirms a single shared teardown.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_IsIdempotent`	3/3 killed	Confirms exactly one transport close and one completion despite three Dispose calls, plus a fast-return bound.	—
A (90–100)	new `MtpServerClientInProcessTests. Dispose_CallbackIgnoresShutdown_ ReturnsWithinTheDocumentedBound`	2/2 killed	Verifies both the documented abandonment time bound and the logged "abandoning" diagnostic.	—
A (90–100)	new `MtpServerClientInProcessTests. RunTestsAsync_Canceled_ SendsCancelRequestToTheHostedApplication`	2/2 killed	Confirms both the client-side cancellation exception and the wire-level cancel notification reaching the fake server.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_MultipleRequestsOnOneSession_ ReuseTheSameConnection`	3/3 killed	Checks keep-alive negotiation, single-connection reuse count, and total request count together.	—
A (90–100)	new `MtpServerClientInProcessTests. LaunchInProcessAsync_IgnoresEnvironmentVariablesAndWarns`	1/1 killed	Asserts the specific warning content naming the ignored option.	—
A (90–100)	new `MtpServerClientInProcessAcceptanceTests. InProcessHost_ DiscoversAndRunsItsOwnMSTestNodes_ WithoutStartingAProcess`	5/5 killed	Real end-to-end embedded-host run asserting build success, exit code, and each marker line the generated app emits.	—
A (90–100)	mod `MtpServerClientAcceptanceTests. DiscoverAndRun_ViaSourcePackageClient_ ReportsExpectedTestNode`	1/1 killed	New lines assert the external-process ShutdownAsync/Dispose share a teardown and report an exit code.	—
A (90–100)	mod `MtpServerClientSourcePackageConsumerTests. HostileConsumer_ CompilesAgainstPackedSource`	N/A	Compile-only guard; added lines correctly extend surface coverage to `ShutdownAsync` and the in-process launch path.	—

_{This advisory comment was generated automatically. Grades are heuristic

and informational — they do not block merging. Suggestions on the Files

changed tab can be applied with one click. Re-run with
/review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · [◷]( · ◷)

github-actions · 2026-08-31T19:32:24Z

🧵 Parallel-safety audit — PR #10898

Parallelization — audited assemblies:

Test assembly	Scope	Workers	Analyzer coverage
`MSTest.Acceptance.IntegrationTests`	`MethodLevel` (default `[assembly: Parallelize(Scope = ExecutionScope.MethodLevel, Workers = 0)]` in `Program.cs`)	CPU count	coverable once the parallel-safety analyzers ship (attribute-based opt-in)
`Microsoft.Testing.Platform.Acceptance.IntegrationTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped
`Microsoft.Testing.Platform.ServerMode.Client.Sources.UnitTests`	`MethodLevel` (`Program.cs`)	CPU count	coverable once shipped

This PR did not touch any .runsettings/testconfig.json/Directory.Build.* parallelization config — no assembly's scope changed.

Findings: A (global-state) 0 · B (paths) 0 · C (declaration) 0 · D (over-serialization) 0 — by severity: Critical 0 · High 0 · Warning 0 · Info 0.

No unsafe call sites found in the lines this PR added or modified.

MtpServerClientAcceptanceTests.cs — added lines only call client.ShutdownAsync() / assert ServerExitCode; no process-global state touched.
MtpServerClientSourcePackageConsumerTests.cs — added lines call into an isolated generated test asset (DriveAsync/DriveInProcessAsync); no shared/relative paths, env vars, culture, or console state mutated at the assembly level (Console.WriteLine here writes to the generated app's own redirected output, not the auditing process).
MtpServerClientInProcessAcceptanceTests.cs (new) — uses TestAsset.GenerateAssetAsync, which allocates a unique GUID-suffixed TempDirectory per instance (TestAsset.cs:19-26, throws if a path already exists) — no shared-path collision. CreateChildEnvironment() builds a fresh Dictionary passed only to the spawned child process's environment, not Environment.SetEnvironmentVariable, so it does not mutate this process's state.
FakeMtpServer.cs / MtpServerClientInProcessTests.cs (new) — each test constructs its own TcpListener/FakeMtpServer bound to an OS-assigned loopback port (new TcpListener(IPAddress.Loopback, 0)), so there is no fixed-port collision between concurrently-running tests; no static/process-global state is written.

Nothing here implies a testability or smell concern beyond this audit's scope.

_{Advisory only — heuristic, non-blocking. Re-run with /parallel-audit. This audit answers "is it parallel-safe?"; for testability, smells, or flakiness see the detect-static-dependencies / test-smell-detection / test-anti-patterns analyses.}

🤖 Automated content by GitHub Copilot. Generated by the Parallel-safety audit on PR (on open / sync) workflow. · auto · 117.1 AIC · ⌖ 3.53 AIC · ⊞ 24.8K · [◷]( · ◷)

github-actions

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 159.6 AIC · ⌖ 2.87 AIC · ⊞ 16.9K · ◷

github-actions · 2026-08-31T19:32:25Z

+ client.Dispose();
+
+ Assert.IsTrue(server.Completion.IsFaulted);
+ _ = server.Completion.Exception;


🧪 Test review · Grade B (80–89) — Reads server.Completion.Exception but never asserts on it, so a mutation changing the faulted exception's identity/message would survive.
Assert the captured exception's message/type instead of discarding it after touching .Exception.
Suggested change
_=server.Completion.Exception;
Exception?faultException=server.Completion.Exception?.GetBaseException();
Assert.IsInstanceOfType<InvalidOperationException>(faultException);
Assert.AreEqual("The application failed while shutting down.",faultException!.Message);

github-actions · 2026-08-31T19:32:25Z

+ ? typed
+ : SerializerUtilities.Deserialize<InitializeRequestArgs>((IDictionary<string, object?>)initialize.Params!);
+
+ Assert.AreEqual(isStateful, args.Capabilities.IsStateful, "The in-process path must forward the client's stateful capability unchanged.");


🧪 Test review · Grade B (80–89) — Only asserts the sent initialize args; a mutation dropping the negotiated round-trip value on client.Capabilities is not caught.
Also assert client.Capabilities.IsStateful reflects the negotiated round-trip value, not only the sent request.
Suggested change
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
Assert.AreEqual(isStateful,capabilities.IsStateful,"The negotiated capability the client stores must reflect the server's round trip, not just the sent request.");

github-actions

Note

🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

✅ 22/22 dimensions clean — no findings.

Summary: This PR adds MtpServerClient.LaunchInProcessAsync for embedded hosts (MAUI, Android/iOS) that cannot Process.Start. The implementation is thorough and well-structured:

Threading & Concurrency: Shutdown is idempotent via lock + lazy Task, Volatile.Read/Write for cross-thread exit code, Task.Run avoids sync-context deadlocks. No shared mutable state without synchronization.
Security & IPC: Loopback TCP only, no command-line injection surface (arguments are array-based), WASM fails fast with PlatformNotSupportedException.
Public API: All new API is internal (source-only package). No init accessors. No PublicAPI.Unshipped.txt changes needed.
Performance: GetCurrentProcessId() snapshotted once in constructor. No hot-path allocations. WaitBoundedAsync clamps timeouts correctly.
Cross-TFM: #if NET8_0_OR_GREATER guard on AcceptTcpClientAsync(CancellationToken). RuntimeInformation used for OS detection on net462.
Resource Management: Every disposable (TcpListener, TcpClient, CancellationTokenSource, Process) has cleanup in both success and error paths. Pending accepts are neutralized. ServerCancellation is only disposed when safe.
Defensive Coding: Callback exceptions are wrapped as InnerException in MtpServerConnectionClosedException. Null task from callback is caught. Cancel() runs on thread pool to avoid blocking registration.
Error Handling: Shared teardown task wrapped in catch-all so it never faults. ObserveFailure prevents UnobservedTaskException. Every teardown helper is non-throwing.
Tests: 68 unit tests + acceptance tests covering argument shape, lifecycle, faults, cancellation, timeouts, idempotent disposal, notification-handler disposal, and the real end-to-end in-process path.
Documentation: Changelog, protocol intro, PACKAGE.md all updated. XML doc comments are thorough.

The refactoring of shared transport logic into MtpServerConnector and the IMtpServerHost abstraction is clean and prevents the two launch paths from drifting. The existing LaunchAsync(path) path is provably unchanged (same argument string, same failure messages, same teardown order).

Copilot

Copilot review overview

Review tier: Balanced
Findings: 3

New issues introduced by this change (3)

Severity	Finding
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `MtpJsonRpcConnection.Dispose()` closes the socket and then waits up to its 5-second read-loop…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — `ShutdownAsync` writes this nullable property from the teardown worker while callers can read…
	`src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerInProcessHost.cs` — A canceled callback takes this branch without an inner exception, although the package…

What changed in this PR

Adds in-process launch support to the source-only MTP server-mode client for embedded/mobile hosts, addressing #10890 and enabling scenarios related to #9809.

Changes:

Adds callback-based in-process hosting with shared transport setup.
Adds bounded asynchronous shutdown and server exit-code reporting.
Adds documentation, unit coverage, and real-MSTest acceptance coverage.

File	Description
`MtpServerClientInProcessTests.cs`	Tests in-process lifecycle and protocol behavior.
`FakeMtpServer.cs`	Supports server-to-client dial-back mode.
`MtpServerClientInProcessAcceptanceTests.cs`	Exercises a real in-process MSTest application.
`MtpServerClientAcceptanceTests.cs`	Covers external-process shutdown and exit code.
`MtpServerClientSourcePackageConsumerTests.cs`	Extends source-package compile coverage.
`PACKAGE.md`	Documents embedded-host usage and lifecycle.
`MtpServerProcess.cs`	Adopts shared host and shutdown abstractions.
`MtpServerInProcessHost.cs`	Implements callback hosting and teardown.
`MtpServerConnector.cs`	Centralizes listener and transport setup.
`MtpServerClientOptions.cs`	Adds the shutdown timeout option.
`MtpServerClientExceptions.cs`	Supports preserved inner exceptions.
`MtpServerClient.cs`	Exposes in-process launch and shutdown.
`IMtpServerHost.cs`	Defines common host ownership behavior.
`IMtpServerClient.cs`	Adds shutdown and exit-code members.
`001-protocol-intro.md`	Documents reference-client launch modes.
`Changelog-Platform.md`	Records the new embedded-host capability.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+ Connection.Dispose();
+ SafeDispose(_client, _logger, "Disposing the accepted client socket");
+ MtpServerConnector.SafeStop(_listener, _logger);
+
+ bool stopped = await ShutdownServerAsync(_serverTask, _serverCancellation, _shutdownTimeout, _logger).ConfigureAwait(false);


+ /// Gets the exit code the hosted application returned, or <see langword="null"/> while it is still
+ /// running (or when it failed or was abandoned rather than returning one).
+ /// </summary>
+ public int? ExitCode { get; private set; }


+ if (serverTask.IsCanceled)
+ {
+ return new MtpServerConnectionClosedException(
+ "The in-process Microsoft.Testing.Platform application was canceled before connecting back.");
+ }


Amaury Levé (Evangelink)and others added 4 commits August 31, 2026 19:11

CopilotAI balanced review requested due to automatic review settings August 31, 2026 19:22

Copilotstarted reviewing on behalf of Amaury Levé (Evangelink)August 31, 2026 19:23 View session

github-code-qualityBot found potential problems Aug 31, 2026
View reviewed changes

github-actionsBot reviewed Aug 31, 2026
View reviewed changes

CopilotAI reviewed Aug 31, 2026
View reviewed changes

Amaury Levé (Evangelink)and others added 4 commits August 31, 2026 19:11

CopilotAI balanced review requested due to automatic review settings August 31, 2026 19:22

Copilotstarted reviewing on behalf of Amaury Levé (Evangelink)August 31, 2026 19:23 View session

github-code-qualityBot found potential problems Aug 31, 2026
View reviewed changes

github-actionsBot reviewed Aug 31, 2026
View reviewed changes

CopilotAI reviewed Aug 31, 2026
View reviewed changes

Amaury Levé (Evangelink)and others added 4 commits August 31, 2026 19:11

CopilotAI balanced review requested due to automatic review settings August 31, 2026 19:22

Copilotstarted reviewing on behalf of Amaury Levé (Evangelink)August 31, 2026 19:23 View session

github-code-qualityBot found potential problems Aug 31, 2026
View reviewed changes

github-actionsBot reviewed Aug 31, 2026
View reviewed changes

CopilotAI reviewed Aug 31, 2026
View reviewed changes

Amaury Levé (Evangelink)and others added 4 commits August 31, 2026 19:11

CopilotAI balanced review requested due to automatic review settings August 31, 2026 19:22

Copilotstarted reviewing on behalf of Amaury Levé (Evangelink)August 31, 2026 19:23 View session

github-code-qualityBot found potential problems Aug 31, 2026
View reviewed changes

github-actionsBot reviewed Aug 31, 2026
View reviewed changes

CopilotAI reviewed Aug 31, 2026
View reviewed changes

Amaury Levé (Evangelink)and others added 4 commits August 31, 2026 19:11

CopilotAI balanced review requested due to automatic review settings August 31, 2026 19:22

Copilotstarted reviewing on behalf of Amaury Levé (Evangelink)August 31, 2026 19:23 View session

github-code-qualityBot found potential problems Aug 31, 2026
View reviewed changes

github-actionsBot reviewed Aug 31, 2026
View reviewed changes

CopilotAI reviewed Aug 31, 2026
View reviewed changes

Amaury Levé (Evangelink)and others added 4 commits August 31, 2026 19:11

CopilotAI balanced review requested due to automatic review settings August 31, 2026 19:22

Copilotstarted reviewing on behalf of Amaury Levé (Evangelink)August 31, 2026 19:23 View session

github-code-qualityBot found potential problems Aug 31, 2026
View reviewed changes

github-actionsBot reviewed Aug 31, 2026
View reviewed changes

CopilotAI reviewed Aug 31, 2026
View reviewed changes

Amaury Levé (Evangelink)and others added 4 commits August 31, 2026 19:11

CopilotAI balanced review requested due to automatic review settings August 31, 2026 19:22

Copilotstarted reviewing on behalf of Amaury Levé (Evangelink)August 31, 2026 19:23 View session

github-code-qualityBot found potential problems Aug 31, 2026
View reviewed changes

github-actionsBot reviewed Aug 31, 2026
View reviewed changes

CopilotAI reviewed Aug 31, 2026
View reviewed changes

Amaury Levé (Evangelink)and others added 4 commits August 31, 2026 19:11

CopilotAI balanced review requested due to automatic review settings August 31, 2026 19:22

Copilotstarted reviewing on behalf of Amaury Levé (Evangelink)August 31, 2026 19:23 View session

github-code-qualityBot found potential problems Aug 31, 2026
View reviewed changes

github-actionsBot reviewed Aug 31, 2026
View reviewed changes

CopilotAI reviewed Aug 31, 2026
View reviewed changes

-_=server.Completion.Exception;
+Exception?faultException=server.Completion.Exception?.GetBaseException();
+Assert.IsInstanceOfType<InvalidOperationException>(faultException);
+Assert.AreEqual("The application failed while shutting down.",faultException!.Message);

	Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
	Assert.AreEqual(isStateful,args.Capabilities.IsStateful,"The in-process path must forward the client's stateful capability unchanged.");
	Assert.AreEqual(isStateful,capabilities.IsStateful,"The negotiated capability the client stores must reflect the server's round trip, not just the sent request.");

Uh oh!

Conversation

Amaury Levé (Evangelink) commented Aug 31, 2026

Why

What

Behavior worth reviewing

Tests

Review history

Open design questions

Uh oh!

github-actionsBot commented Aug 31, 2026

🧪 Expert test review — PR #10898

Uh oh!

github-actionsBot commented Aug 31, 2026

🧵 Parallel-safety audit — PR #10898

Uh oh!

github-actionsBot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actionsBotAug 31, 2026

Choose a reason for hiding this comment

Uh oh!

github-actionsBotAug 31, 2026

Choose a reason for hiding this comment

Uh oh!

github-actionsBot left a comment

Choose a reason for hiding this comment

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Amaury Levé (Evangelink) commented Aug 31, 2026

Why

What

Behavior worth reviewing

Tests

Review history

Open design questions

Uh oh!

github-actionsBot commented Aug 31, 2026

🧪 Expert test review — PR #10898

Uh oh!

github-actionsBot commented Aug 31, 2026

🧵 Parallel-safety audit — PR #10898

Uh oh!

github-actionsBot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actionsBotAug 31, 2026

Choose a reason for hiding this comment

Uh oh!

github-actionsBotAug 31, 2026

Choose a reason for hiding this comment

Uh oh!

github-actionsBot left a comment

Choose a reason for hiding this comment

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants