Repository navigation
[tests] Use .NET 11 Process APIs for live device process output - #13519
simonrozsival wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
StopAsync returns early when the root process has already exited, leaving surviving descendants untracked and unterminated in callers that await root exit before cleanup.
1 open issue
What changed in this PR
Extends #12985’s host-process fixes to live device tests and Microsoft.Android.Run, addressing the process-lifetime audit in #12981.
Changes:
- Replaces output callbacks with owned raw readers and separates execution, shutdown, and output-drain deadlines.
- Adds bounded process cleanup while removing fixture-wide shutdown of shared ADB servers.
- Adds Unix regression tests for inherited pipes, diagnostic tails, Ctrl+C, escalation, and consumer failures.
| File | Description |
|---|---|
tests/Xamarin.Android.Tools.AndroidSdk-Tests/Xamarin.Android.Tools.AndroidSdk-Tests.csproj |
Links cleanup helpers and builds the run executable for regression tests. |
tests/Xamarin.Android.Tools.AndroidSdk-Tests/DeviceProcessTests.cs |
Adds process-lifetime and diagnostic-preservation regressions. |
tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs |
Migrates live run, test, watch, template, and benchmark scenarios to asynchronous capture and cleanup. |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Utilities/ProcessExtensions.cs |
Adds bounded graceful shutdown, escalation, and descendant discovery. |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/DeviceTest.Logcat.cs |
Adds asynchronous logcat monitoring with bounded cleanup and reader draining. |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/DeviceTest.cs |
Routes live logcat monitoring through the new helper. |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/BaseTest.cs |
Removes fixture-wide ADB server termination. |
src/Microsoft.Android.Run/Program.cs |
Separates client exit from output completion and coordinates Ctrl+C cleanup. |
.github/skills/tests/references/test-catalog.md |
Documents the new regression suite and remaining platform/device coverage. |
Review effort: Balanced
ef04adc to
b637988
Compare
Context: #12981 Context: https://devblogs.microsoft.com/dotnet/process-api-improvements-in-dotnet-11/ Replace `OutputDataReceived`/`ErrorDataReceived` + `Begin*ReadLine ()` with `Process.ReadAllLinesAsync ()`, which drains stdout and stderr concurrently, in `Microsoft.Android.Run` (instrumentation and logcat), `DeviceTest.MonitorAdbLogcat ()` and the `InstallAndRunTests` `dotnet run`/`test`/`watch` consumers. Finite commands are bounded by cancelling the read instead of polling `WaitForExit (int)` and then calling the unbounded `WaitForExit ()` to drain events. Live commands bound the post-kill output drain. `ProcessExtensions.SendCtrlC ()` uses `SafeProcessHandle.Signal (PosixSignal.SIGINT)` and `Process.RunAndCaptureText ()` instead of a libc `kill` P/Invoke and a manual `pgrep` capture. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
b637988 to
c25fdb6
Compare
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Needs Changes
Reviewed all four changed source files in full, the diff against the actual stacked PR base, callers, target-framework configuration, and the runtime implementations of the new APIs.
Findings: 2 ❌ errors, 1
- Instrumentation output draining loses Ctrl+C cancellation after process exit when an inherited pipe remains open.
- The finite tests' deadlines no longer cover process exit after output reaches EOF.
- Timed reader waits can leave a background writer running while test output is read and resources are disposed.
The API migration usefully serializes stdout/stderr consumption, and redirecting logcat stderr satisfies the new reader's requirements. The remaining issues are in cancellation and ownership around those readers, rather than the API choice itself.
CI: All 40 reported GitHub checks succeeded, including the aggregate dotnet-android check for Azure build 1679365. Independent Azure build/timeline verification was blocked because az devops invoke required authentication. No local .NET 11/device execution was performed; the installed SDKs stop at .NET 10.
Generated by Android PR Reviewer for #13519 · copilot · gpt61 · 68.5 AIC · ⌖ 5.01 AIC · ⊞ 20.2K
Comment /review to run again
Address review feedback on #13519: * Microsoft.Android.Run: make the instrumentation output reader cancellable and observe it on every exit path. * ProcessExtensions: add StopAndDrainOutput and WaitForExitAndOutput, which share a bounded deadline for exit and output draining, and track descendant processes so they are killed even after the root process exits. * DotNetCLI: track descendants for StartRun and StartTest. * InstallAndRunTests: use cancellable, synchronized readers and drain output before taking snapshots. * Add regression tests for descendant cleanup after root exit and for timeouts after output streams close. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "169.254.169.254"See Network Configuration for more information.
|
There was a problem hiding this comment.
⚠️ Needs Changes
Reviewed all seven changed files in full, their callers and project configuration, and the existing resolved review threads. 1 ❌ error and 1
The unified output readers and explicit process/output deadline are good improvements. The two new host regressions cover inherited pipes after root exit and processes that close their output before exiting; the earlier cancellation and output-snapshot findings are addressed.
CI: GitHub reports all 40 checks successful, including the overall dotnet-android check for build 1679876. Independent Azure build-result verification was blocked by the CLI's authentication requirement, so I cannot independently confirm the Azure result. The focused command dotnet test tests/Xamarin.Android.Tools.AndroidSdk-Tests/Xamarin.Android.Tools.AndroidSdk-Tests.csproj --no-restore --filter 'FullyQualifiedName~StreamingTests.ProcessTimeout' -v minimal failed during .NET CLI initialization with DirectoryNotFoundException, before any tests executed. No source changes were made.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
169.254.169.254
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "169.254.169.254"See Network Configuration for more information.
Generated by Android PR Reviewer for #13519 · copilot · gpt61 · 60.5 AIC · ⌖ 7.48 AIC · ⊞ 20.3K
Comment /review to run again
| async Task TrackDescendantsAsync () | ||
| { | ||
| while (!cts.IsCancellationRequested && !process.HasExited) { | ||
| var pids = new List<int> (); |
There was a problem hiding this comment.
🤖 ❌ Resource management — Polling does not retain ownership of descendants across a fast parent exit. TrackDescendants schedules its first scan with Task.Run, so a root that starts a child and exits before that task runs leaves handles empty; a short-lived intermediate process can also disappear between scans and reparent its children. Once that happens, Kill(entireProcessTree: true) cannot recover those descendants and StopAndDrainOutput only cancels the reader, leaving the child running. The new regression avoids this race by keeping the root alive with sleep 1. Please establish ownership at launch using a process group/job or an equivalent lifecycle mechanism, and cover a root/intermediate that exits immediately after spawning its child.
Rule: Deterministic resource cleanup; regression tests must exercise the failure boundary.
| } finally { | ||
| cts.Cancel (); | ||
| await ObserveOutputTaskAsync (); | ||
| await logcatTask; |
There was a problem hiding this comment.
🤖 outputTask skips the rest of this cleanup. For example, if the stream reader faults with an I/O exception, the earlier await outputTask enters this finally, but ObserveOutputTaskAsync rethrows that same fault because it only handles cancellation. Execution then never reaches await logcatTask or the logcat kill/reap block, leaving an already-started adb logcat running after the command fails. Put the reader observation inside a nested try/finally so logcat cleanup still runs while preserving the original exception.
Rule: Exception-safe cleanup of owned processes.
Context: #12981
Context: https://devblogs.microsoft.com/dotnet/process-api-improvements-in-dotnet-11/
Context: #12985
Context: #12988
.NET 11 adds
ProcessAPIs that cover what our device-test harness andMicrosoft.Android.Runhand-roll today.Process.ReadAllLinesAsync ()drains stdout and stderr concurrently into a single ordered stream ofProcessOutputLinevalues.SafeProcessHandle.Signal ()sends POSIX signals.Process.RunAndCaptureText ()runs a finite command and captures its output.The live
dotnet run/test/watchconsumers inInstallAndRunTests,DeviceTest.MonitorAdbLogcat ()andMicrosoft.Android.Run's instrumentation and logcat streaming still use theOutputDataReceived/ErrorDataReceived+Begin*ReadLine ()pattern. That pattern has two lifetime problems:StringBuilders and console writes.Process.WaitForExit (). The finite tests polledWaitForExit (int)and then called the parameterlessWaitForExit ()"to ensure async output events are fully drained".MonitorAdbLogcat ()calledKill (), thenWaitForExit (), then waited on aManualResetEventSlimfor thenullEOF event. All of these waits are unbounded. Process exit is not EOF: if any descendant inherited the pipe, the test hangs until the NUnit/CI timeout.ProcessExtensions.SendCtrlC ()also P/Invoked libckill (pid, SIGINT)and decodederrnoby hand. It collected descendants with a manualProcess.Start ("pgrep")+ReadToEnd ()+WaitForExit ().This is a targeted port to the new APIs, with no new helpers, runners or timeout knobs:
ReadAllLinesAsync (), andline.StandardErrorselects the stream. Live commands consume it on a reader task. Finite commands, namely thedotnet new androidtestrun/test cases, BenchmarkDotNet and logcat monitoring, enumerate it directly with aCancellationTokenSourcedeadline. Cancellation replaces polling, and the owned tree is then killed.Microsoft.Android.Runawaits the instrumentation reader before cleaning up logcat and before inspecting output withGetInstrumentationFailure (). The oldWaitForExitAsync ()drained implicitly before that point.SendCtrlC ()usesSafeProcessHandle.TryOpen ()+Signal (PosixSignal.SIGINT)for descendants andprocess.SafeHandle.Signal ()for the root.TryOpen ()failing covers the "already exited" race thatESRCHused to cover.pgrepnow runs viaProcess.RunAndCaptureText ().MonitorAdbLogcat ()now redirects stderr as well, becauseReadAllLinesAsync ()requires both streams to be redirected and throwsInvalidOperationException: StandardError has not been redirectedotherwise.A note on the API choice: in the synchronous
MonitorAdbLogcat (), the stream is consumed viaReadAllLinesAsync (token).ToBlockingEnumerable ()rather thanReadAllLines (TimeSpan?). On the current runtime, the synchronous overload did not yield a line that had already been written before its timeout expired:echo a; sleep 5with a 1 s timeout produced onlyTimeoutException. The async enumerator yieldedaafter about 48 ms, then threwOperationCanceledExceptionat the deadline.AdbHelper/AndroidTestAdapterare unchanged; #12988 migrates their finite commands.Testing
dotnet build src/Microsoft.Android.Run/Microsoft.Android.Run.csproj --no-restore: succeededdotnet build src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Xamarin.ProjectTools.csproj --no-restore: succeededdotnet build src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj --no-restore: succeededdotnet build tests/MSBuildDeviceIntegration/MSBuildDeviceIntegration.csproj --no-restore: succeededSendCtrlC ()logic against a realshprocess tree: the root trapped SIGINT and exited with 130. A background grandchild that ignores SIGINT kept the pipe open, and the bounded 30 s reader wait returned instead of hanging.InstallAndRunTests(no device attached) and Windows.SendCtrlC ()remains Unix-only, as before.