Repository navigation
[build] Use native Process APIs in bootstrap tasks - #13516
Merged
jonathanpeppers merged 9 commits intoNov 13, 2026
Merged
Conversation
Related to #12981. Bootstrap ADB waits can outlive both deadlines because the unlimited WaitForExit also drains inherited pipes. Its output callbacks can concurrently mutate parsers and merged writers. Use .NET 11 multiplexed reads with separate root and EOF deadlines. Retain timeout diagnostics and cancel only the finite ADB client. Include invocations and retry delays in the emulator boot deadline. Run parallel commands with observed CPU-count-bounded workers and execution-thread logging. Join cleanup before returning, including when logging fails. Preserve raw arguments and quote managed commands. Release only recorded emulator PIDs. Bound the wait without requiring a cross-task-host Unix exit status. Keep explicit adb kill-server teardown, but remove repeated signals and inferred emulator targets. Validation: 179 host tests passed, with 4 existing platform skips. Ordinary and strict -m -mt bootstrap owner builds passed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
7 tasks
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Output capture can lose unterminated diagnostics when inherited pipes reach the deadline, and concurrent log enumeration can make the cancellation test flaky.
2 open issues
What changed in this PR
This PR addresses part of #12981 by separating process exit from output completion in bootstrap tooling, reducing hangs during ADB commands, parallel execution, and emulator cleanup.
Changes:
- Adds separate process/output deadlines, cancellation handling, and serialized ADB output consumption.
- Observes parallel command results and joins cleanup before returning.
- Tightens target readiness and emulator teardown, with POSIX subprocess regression tests.
| File | Description |
|---|---|
tests/Microsoft.Android.Build.BaseTasks-Tests/Microsoft.Android.Build.BaseTasks-Tests.csproj |
References bootstrap tasks and copies the release target for integration tests. |
tests/Microsoft.Android.Build.BaseTasks-Tests/BootstrapProcessTests.cs |
Adds subprocess timeout, cancellation, output, and cleanup tests. |
build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/WaitForAndroidEmulator.cs |
Shares a deadline across readiness commands and retry delays. |
build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/RunParallelCmds.cs |
Collects command results and bounds output draining and cleanup. |
build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/KillProcess.cs |
Bounds recorded-PID termination waits and handles exited processes. |
build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/CheckAdbTarget.cs |
Rejects unsuccessful probes and respects cancellation. |
build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/Adb.cs |
Separates process/output lifetimes and centralizes output consumption. |
build-tools/scripts/TestApks.targets |
Restricts emulator teardown to recorded ownership and removes repeated raw signals. |
Review effort: Balanced
Context: #12981 ReadAllLinesAsync only delivers an unterminated line at EOF. If a descendant retains the output pipes, the output deadline cancels the read before EOF and drops both diagnostic tails. The original policy cases used newline-terminated output or killed the only writer, so they missed this loss. Read both pipes incrementally and decode each completed byte read, preserving configured encodings and BOM detection. Keep ADB line parsing on the executing thread; canceled fragments belong in diagnostics and output files, not complete parser records. RunParallelCmds retains the raw text from both streams. Process deadlines, bounded reader cleanup, and root-only ADB/shared-server ownership remain unchanged. Extend the existing owner tests with retained-writer tails at command, output, and cancellation deadlines, separate output files, multibyte diagnostics, split BOMs, and line terminators. Allow the phase-specific readiness fixture enough time to start before checking the PM deadline. Validation: 186 host tests passed, with 4 existing platform skips. Ordinary and strict -m -mt bootstrap owner builds passed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context: #12981 The recorded-PID regression starts two subprocesses before its cleanup scope, waits indefinitely for one, and does not observe termination of the other on assertion failure. Nest ownership scopes immediately after each launch so a later launch or assertion cannot bypass cleanup. Terminate and join both fixtures with bounded waits before disposing their handles. Reuse the same fixture cleanup for recorded PID files and the release-target emulator. Validation: 186 host tests passed, with 4 existing platform skips. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context: #12981 The shared Windows cleanup step stops every nunit3-console, MSBuild, and adb process by name. Neither the template nor its pool-based callers establishes launch membership or an exclusive-machine precondition, so this cleanup can stop another job or a shared server. Remove the sweep and its active template references. Keep explicit per-owner process cleanup and ReleaseAndroidTarget server teardown unchanged; pool selection is not evidence that an arbitrary process belongs to this invocation. Validation: affected YAML parses and no references to the retired template remain. No process-name sweep was executed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context: #12981 WorkItemTimeout bounds a remote Helix work item, not the host submission workers waiting for it. The matrix launcher has no local deadline or cleanup scope, so a later launch, logging failure, or cancellation can leave previously launched workers running. Prepare scripts before launching workers, record each successful launch immediately, and apply a separate configurable one-hour host deadline. Always stop, join, and dispose the owned workers, preserving file-directed diagnostics and every exit status. On Windows PowerShell, terminate only the recorded worker tree with a bounded PID-specific taskkill; do not require newer runtime APIs or sweep process names. Extend the existing Helix environment tests with real owned worker processes for mixed results, partial launch failure, logger failure, timeout/tree cleanup, and unrelated-process survival. Expected fixture errors stay out of Azure task logging. Tests pass on PowerShell 7.5.2/macOS; Windows PowerShell and live Helix execution remain unvalidated. Affected YAML and inline PowerShell parse successfully. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context: #12981 Context: #12986 Native Process capture sets redirected readers to SyncMode, but Process.Close deliberately leaves readers in that mode for their caller to dispose. On the pinned 12.0.0-alpha.1.26556.115 runtime, 100 actual Which version queries retain 200 pipe descriptors until garbage collection. Dispose both readers explicitly in the CI collector, GenAPI/ApiCompat capture, version discovery, and git metadata capture. Keep native capture ahead of the reader-property accesses so validation does not reject mixed read modes. Replace the release fixture static capture with a started instance, bounded root/output handling, and explicit reader disposal. The raw ADB/parallel decoder policies and inherited-console enum helpers are unchanged. Add a no-GC owner regression for successful and failing version queries: both retain zero descriptors after 100 commands on the exact pinned runtime. Validation: 188 host tests passed, with 4 existing platform skips; ordinary and strict bootstrap builds passed. The CI script compiles and its offline argument-validation contract passes. Publication remains held. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context: #12981 The full pinned-runtime run exposed retained-output cases expiring their one-second command budget before the fixture emitted any output; the same cases passed in isolation. Give the existing command timeout enough startup allowance to reach the pipe-retention scenario rather than testing host scheduling. Keep the one-second EOF deadline and finite total ceiling, assert that the fixture actually started, and verify that root-timeout cases consume their configured command budget before the output-drain period. Production timeouts and the raw decoder policy are unchanged. Validation: full pinned host suite passed 188 tests with 4 existing skips, including zero retained descriptors in both no-GC version-probe cases. Ordinary and strict bootstrap builds passed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
marked this pull request as draft
November 12, 2026 21:13
Context: https://devblogs.microsoft.com/dotnet/process-api-improvements-in-dotnet-11/ Related to #12981. Replace the expanded process-lifetime implementation with a targeted API migration. Restore the unrelated CI, emulator, cleanup, parser-policy, and prep-tool changes, and remove the custom output decoder. Use native ReadAllLinesAsync for bootstrap ADB output and SafeProcessHandle waits for its existing process timeout. Keep the existing output timeout, command generation, stream routing, encodings, environment, arguments, and exit-code policy. RunParallelCmds uses ReadAllLines in its existing CPU-bounded loop, preserving its AsyncTask base and log formatting. Dispose both redirected readers after native capture completes. Add seven focused cases for line/file routing, trailing lines, ignored exit codes, process timeout, and concurrent stdout/stderr capture. Validation: 165 host tests passed, with 4 existing platform skips; ordinary and strict bootstrap builds passed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
marked this pull request as ready for review
November 13, 2026 10:15
Member
Author
|
/review |
Contributor
|
✅ 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.
|
Contributor
There was a problem hiding this comment.
Review: ⚠️ Verification incomplete; no blocking code findings
Findings: 0 ❌ errors, 0
Read all four changed files in full, checked the bootstrap/test target frameworks and ADB subclasses, and compared the implementation with the stated preservation requirements. The refactor removes manual completion events, serializes merged-output handling through one consumer, and explicitly disposes redirected readers. The added cases cover stream/file routing, blank and unterminated lines, ignored exit codes, process timeout, and high-volume output. The inline suggestions strengthen capture-completeness and output-drain cancellation coverage.
CI: All 40 returned GitHub checks report success, including the aggregate dotnet-android check for Azure build 1679374. Independent Azure build/timeline verification was blocked: the build query required authentication and the timeline query failed during CLI initialization. I therefore cannot independently certify the full Azure result.
Local validation: Attempted dotnet test tests/Microsoft.Android.Build.BaseTasks-Tests/Microsoft.Android.Build.BaseTasks-Tests.csproj --no-restore --nologo -v:q --filter FullyQualifiedName~BootstrapProcessTests; the CLI failed before building or running tests because its first-use initialization could not open the required runtime directory. No local test result is claimed. 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 #13516 · copilot · gpt61 · 33.6 AIC · ⌖ 8.06 AIC · ⊞ 19.5K
Comment /review to run again
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
jonathanpeppers
approved these changes
Nov 13, 2026
jonathanpeppers
deleted the
simonrozsival-repository-process-orchestration
branch
November 13, 2026 19:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context: https://devblogs.microsoft.com/dotnet/process-api-improvements-in-dotnet-11/
Related to #12981.
Bootstrap
AdbandRunParallelCmdseach implement stdout/stderr capture with event handlers, output buffers, and process waits. .NET 11 provides multiplexed line readers that drain both streams together, making the event handlers and manual completion events unnecessary.Replace that plumbing with
ReadAllLinesAsync()inAdbandReadAllLines()inRunParallelCmds, and use the nativeSafeProcessHandlewait methods. Keep ADB's process/output timeout settings, stream routing, file output, encodings, environment, arguments, and ignored-exit policy. Keep the parallel task's existingAsyncTaskbase, CPU-count concurrency, command construction, and log formatting. Explicitly dispose both redirected readers after capture completes.Production changes are confined to these two task files. There is no custom output decoder, new runner abstraction, or CI/emulator/cleanup-policy change. Seven focused cases exercise line/file routing, trailing lines, ignored exit codes, process timeout, and simultaneous stdout/stderr capture.
Validation on macOS:
dotnet test tests/Microsoft.Android.Build.BaseTasks-Tests/Microsoft.Android.Build.BaseTasks-Tests.csproj --no-restore --nologo -v:q— 165 passed, 4 existing skips.dotnet build build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks.csproj --no-restore --nologo -v:q— passed, 0 warnings/errors.dotnet build build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks.csproj --no-restore --nologo -v:q -m -mt -nr:false -warnaserror:MSB4286,MSB4287,MSB4288— passed, 0 warnings/errors.The focused subprocess fixtures require a POSIX shell; Windows execution was not validated.