Repository navigation
[Xamarin.Android.Build.Tasks] Retire finite process facade - #13518
Closed
simonrozsival wants to merge 8 commits into
Closed
simonrozsival wants to merge 8 commits into
simonrozsival wants to merge 8 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Timed-out lint probes can leave child JVMs running on Windows, while undisposed redirected readers during native-library stripping risk exhausting file handles in large builds.
2 open issues
What changed in this PR
Retires the finite-process helper in .NET for Android build tooling, advancing #12981 while leaving persistent daemon and emulator execution unchanged.
Changes:
- Removes
MonoAndroidHelper.RunProcessand migrates callers to native process APIs with structured arguments and timeouts. - Preserves completed emulator names and strip diagnostics on inherited-pipe timeouts, and removes failed strip outputs.
- Adds regression coverage for output draining, argument handling, timeouts, and cleanup.
| File | Description |
|---|---|
src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs |
Removes the process facade and migrates the uname probe. |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/ResourceData.cs |
Uses bounded native capture for keytool. |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/GetAvailableAndroidDevicesTests.cs |
Tests discovery output, structured arguments, and inherited pipes. |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/NativeAotBuildTests.cs |
Tests strip diagnostics, timeouts, and partial-output removal. |
src/Xamarin.Android.Build.Tasks/Tasks/ValidateJavaVersion.cs |
Migrates Java version capture with a timeout. |
src/Xamarin.Android.Build.Tasks/Tasks/ResolveAndroidTooling.cs |
Migrates aapt2 version capture with a timeout. |
src/Xamarin.Android.Build.Tasks/Tasks/Lint.cs |
Migrates lint version capture while preserving environment overrides. |
src/Xamarin.Android.Build.Tasks/Tasks/GetAvailableAndroidDevices.cs |
Bounds discovery probes and preserves completed emulator names. |
src/Xamarin.Android.Build.Tasks/Tasks/CollectNativeFilesForArchive.cs |
Captures both strip streams concurrently and cleans up failed outputs. |
Review effort: Balanced
| }; | ||
| foreach (var pair in environmentVariables) | ||
| startInfo.Environment [pair.Key] = pair.Value; | ||
| var result = Process.RunAndCaptureText (startInfo, TimeSpan.FromSeconds (30)); |
Comment on lines
+160
to
+161
| var output = CaptureDiagnosticsAsync (process.StandardOutput, stdout); | ||
| var error = CaptureDiagnosticsAsync (process.StandardError, stderr); |
This was referenced Nov 12, 2026
Member
Author
|
Blocked validation finding: downstream Azure build 1677420, Windows MSBuild 7, run 46759822/result 100248 ( The available evidence cannot distinguish slow Java startup from retained/inherited pipe EOF or Windows native capture behavior. The lane used cmdline-tools 23.0, JDK 25.0.4.1 and .NET 12.0.0-alpha.1.26556.115. Windows reproduction with that pinned host remains unavailable. Per the user decision, this fix stays blocked: no speculative deadline increase, suppression, CI rerun, or finite source/ref changes. Build: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1677420 . Targeted original build.log/binlog are preserved in the owning session handoff as |
`Xamarin.Android.Tasks.Aapt2Daemon` could block indefinitely while reading a response, waiting for output, or shutting down. A failed worker also left queued jobs without a terminal result. Keep the persistent protocol, but use cancellable async input and stderr reads with a concurrent stdout drain. Bound output-file waits to 30 seconds, observe child exit independently, and terminate only the owned process after a five-second graceful shutdown deadline. Serialize queue/worker lifecycle changes, complete rejected and pending jobs on failure/disposal, and replace stopped MSBuild cache entries. Cancellation restarts the affected daemon without poisoning later commands. Preserve UTF-8 input without a BOM and deferred diagnostic classification. Extend the existing Aapt2Tests owner with protocol/lifetime cases. Both owner builds pass and all 33 Aapt2Tests pass. Full `make all` remains blocked by pre-existing generated Mono.Android binding compile errors observed before this change. Relates to #12981. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context: #13515 The executing .NET 12.0.0-alpha.1.26556.115 runtime leaves accessed synchronous process readers caller-owned. A process handle alone therefore cannot prove that a daemon's redirected pipe handles have closed. Exercise the actual AAPT2 daemon through 100 start/stop cycles in a no-GC region, count real file descriptors, and ensure cleanup does not depend on reader finalization. Warm the process path before measuring and allow a small fixed margin for test-host bookkeeping, rather than growth proportional to daemon launches. The existing daemon already disposes both readers after its producer tasks join; no production fix is needed. On the pinned macOS runtime, the isolated measurement stays at 113 descriptors. Removing just the two reader ownership scopes as a temporary control makes the test fail with 315 descriptors; restoring them returns the measurement to 113. The complete Aapt2Tests suite passes all 34 cases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
force-pushed
the
simonrozsival-aapt2-daemon-lifetime
branch
from
November 12, 2026 16:29
fc35a80 to
7b7fced
Compare
Rely on WaitForExit to drain redirected streams and dispose the cancellation registration before diagnostics and process disposal. Cover output draining, cancellation, start failure, and registration retention on successful and exceptional exits. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve the user-directed reduction to cancellation/draining and registration retention on normal and exceptional completion. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Move finite discovery, version, uname, keytool and native strip calls to native process capture APIs with structured arguments and bounded output collection. Preserve discovery partial results and XA0142 strip diagnostics, including timeout cleanup, without retaining unused event or cancellation callback contracts. Replace obsolete helper lifecycle tests with focused discovery and strip policy regressions. The user-reduced original coverage remains preserved in the prior commits and safety branch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Line-based capture discards unterminated diagnostics when an exited strip process leaves inherited pipes open. Capture both streams as concurrent character chunks, retaining received text at the deadline. Exercise the real timeout with an exited root and fixture-owned orphan holding both streams, including unterminated stdout and stderr. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Wait for the killed inherited-output descendant before deleting its fixture directory. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Drain linker and metadata-tool output concurrently with an independent post-root deadline, preserving MSBuild-controlled active linking and failed-output cleanup. Join readers before disposal and bound live-root termination observation. Use instance native capture with explicit ownership of both redirected readers at finite probe and keytool owners. Cover inherited EOF, real task cancellation, active linking beyond the drain deadline, and discovery pipe release without finalization. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
force-pushed
the
simonrozsival-process-resource-cleanup
branch
from
November 12, 2026 16:39
2ee131d to
b331dd5
Compare
simonrozsival
force-pushed
the
simonrozsival-aapt2-daemon-lifetime
branch
from
November 13, 2026 09:26
7b7fced to
8815d04
Compare
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.
Related to #12981. Stacked on #13515 (
simonrozsival-aapt2-daemon-lifetime,7b7fced0b208fd0570e2d622dcab46f7efedbfc3).Retire the unused
MonoAndroidHelper.RunProcessoverloads and their event, cancellation-registration, and callback machinery. Migrate real finite uname, Java, lint, aapt2-version, emulator-discovery, keytool, and strip callers directly to native process APIs with structured arguments and bounded execution/output draining, preserving diagnostics and error policies. Own both redirected readers explicitly after native capture joins rather than relying on finalization. No replacement process facade or persistent daemon/emulator changes.Keep completed emulator names on inherited-pipe timeout. Strip both streams concurrently as raw character chunks so XA0142 retains unterminated stdout/stderr even when an exited root leaves inherited pipes open; remove partial outputs. Fixture-owned descendants are killed and observed with bounded managed
Process.WaitForExit(5000)before fixture deletion.Bound NativeAOT linker/readobj/objdump output draining independently of root execution. Root exit starts a 30-second EOF deadline; active linking remains controlled by MSBuild cancellation, not an arbitrary linker cap. Join owned readers, bound live-root termination observation, preserve complete-line symbol parsing, partial linker diagnostics, error mappings, failed-link cleanup and atomic metadata output. Before the fix, three established-suite inherited-EOF regressions took 60 seconds and failed; the 31-second active-link control passed.
Local validation at current head
b331dd5c7ef3cdbd91933097c1e0cc3b6c477e20after replaying all six finite commits onto the updated AAPT2 parent:DOTNET_CLI_USE_MSBUILD_SERVER=0 MSBUILDDISABLENODEREUSE=1 ./dotnet-local.sh build src/Microsoft.Android.Build.Tasks/Tests/Microsoft.Android.Build.Tasks.Tests/Microsoft.Android.Build.Tasks.Tests.csproj -c Debug --no-restore -m:1 -nr:false -p:UseSharedCompilation=false -v:q— passed, 0 errors, 0 warnings../dotnet-local.sh test bin/TestDebug/net12.0/Microsoft.Android.Build.Tasks.Tests.dll --filter 'Name~NativeLinkBoundsOnlyPostExitOutputDrain|FullyQualifiedName~ExtractTypeMapKeysFromNativeAotObject' --logger 'trx;LogFileName=finite-restack-modern.trx' --results-directory /Users/simon/.copilot/session-state/46adb309-4abf-4756-91f7-16efbcc103f8/files— passed 205, failed 0, skipped 0. Covers actual 30-second inherited-EOF deadlines, 31-second active linking, real task cancellation with joined asynchronous cleanup, and metadata policies.DOTNET_CLI_USE_MSBUILD_SERVER=0 MSBUILDDISABLENODEREUSE=1 ./dotnet-local.sh build src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj -c Debug --no-restore -m:1 -nr:false -p:UseSharedCompilation=false -v:q— passed, 0 errors, 27 warnings../dotnet-local.sh test bin/TestDebug/net11.0/Xamarin.Android.Build.Tests.dll --filter 'FullyQualifiedName~GetAvailableAndroidDevicesTests|Name~CurrentSdkUsesConfiguredJavaVersionRange|Name~ResolveAndroidToolingCachesAapt2Version|Name~FailedNativeStripReportsBothStreamsAndRemovesPartialOutput' --logger 'trx;LogFileName=finite-restack-owner.trx' --results-directory /Users/simon/.copilot/session-state/46adb309-4abf-4756-91f7-16efbcc103f8/files— passed 64, failed 0, skipped 0. Includes retained raw unterminated strip diagnostics, discovery deadlines/cleanup and 100 actual discovery captures releasing both pipes without GC/finalization.git diff --check— passed.git range-diff fc35a8078238ad4e4de9509f8e1539052f061a0e..c5fa145550a709e666926da983708f729667ee75 7b7fced0b208fd0570e2d622dcab46f7efedbfc3..b331dd5c7ef3cdbd91933097c1e0cc3b6c477e20— all six patches unchanged; stable patch IDs match individually.These are fresh current-head results, superseding the earlier 43-case fixture-only proof at
2ee131d2and 63-case finite-policy proof ate321d01c.Blocked Windows lint finding: downstream Azure build 1677420, run 46759822/result 100248,
CheckLintConfigMerging(NativeAOT)fails with XALNT7017 inProcess.ReadPipes → ReadAllText → RunAndCaptureText → Lint.GetLintVersion, before the actual lint invocation. The binlog records 35.703 seconds in Lint versus this layer's new 30-second whole-capture policy. This is a coupled failure, not classified as unrelated or flaky. Slow Java startup versus retained/inherited pipe EOF versus Windows native capture behavior remains unresolved; Windows reproduction with the pinned host is unavailable. The reader-ownership correction does not claim to fix this timeout: its 30-second policy is unchanged. Per user decision this stays blocked: no guessed deadline increase, suppression, or CI rerun. Detailed evidence.Full SDK preparation remains blocked:
make preparefailed with MSB3231 atsrc/androidsdk/androidsdk.targets:341removing the nonempty sharedsources/android-37.2.stagingdirectory. Shared staging was not repaired or raced; full SDK/application-build and Windows/device coverage are not claimed. No device or ADB use. Protected external worktree dirt is untouched.