EmulatorRunner.LaunchEmulator: return EmulatorLaunchResult with serial, ports, and log path

[Copilot]

LaunchEmulator returned a bare Process, forcing callers to race adb devices to find the serial and hardcode AOSP path conventions to find the log file. All of this data is available to EmulatorRunner at launch time.

Changes

New EmulatorLaunchResult type (Models/EmulatorLaunchResult.cs)

Replaces Process as the return type of LaunchEmulator:

• Process, Pid — process handle and OS PID
• int? ConsolePort, int? AdbPort — populated immediately when pre-assigned, or once PortsResolvedAsync completes
• string? Serial — derived as emulator-{ConsolePort}, null until ports are known
• string LogPath — resolved at launch time
• Task PortsResolvedAsync (internal) — already-completed when ports are pre-assigned; completes when both emulator port-announcement lines are parsed from stdout/stderr; faulted if the process exits before emitting them

Constructor requires Process and logPath (non-null, validated). PortsResolvedAsync is internal set — only EmulatorRunner controls it.

Updated LaunchEmulator signature

public EmulatorLaunchResult LaunchEmulator(
    string avdName,
    bool coldBoot = false,
    int? consolePort = null,   // adds -ports <console>,<adb>; serial known before launch
    int? adbPort = null,       // defaults to consolePort + 1; throws if provided without consolePort
    string? logFile = null,    // adds -logfile; otherwise resolved from env vars
    List<string>? additionalArgs = null)

Pre-assigned ports — when consolePort is provided, -ports console,adb is passed to the emulator and PortsResolvedAsync is already completed, so Serial is known synchronously:

var r = runner.LaunchEmulator("Pixel_7_API_35", consolePort: 5554);
Console.WriteLine(r.Serial);    // "emulator-5554" — no await needed
Console.WriteLine(r.LogPath);   // ~/.android/avd/Pixel_7_API_35.avd/emulator.log

Late-bound ports — when no port is pre-assigned, stdout/stderr are parsed for the stable emulator boot lines and PortsResolvedAsync completes once both are found:

emulator: Listening on port 5554
emulator: ADB Server has started successfully on port 5555

LaunchEmulatorAsync

A companion LaunchEmulatorAsync method awaits PortsResolvedAsync before returning, providing a single awaitable entry point for callers that need ports resolved before proceeding.

Log path resolution (ResolveAvdLogPath)

Priority: explicit -logfile arg → ANDROID_AVD_HOME env var → ANDROID_USER_HOME/avd → ~/.android/avd/<name>.avd/emulator.log. Runner-level environmentVariables overrides are respected. Whitespace-only logFile values fall through to the default.

ProcessUtils.StartFireAndForget (new internal)

Added ProcessUtils.StartFireAndForget(Process, onOutputLine, onErrorLine, onExited) to consolidate fire-and-forget process launching (event-handler setup, start, begin async reads). LaunchEmulator uses it instead of managing those steps inline.

Unix SIGINT isolation

On Unix, the emulator is now launched via sh -c "trap '' INT; exec ..." so the emulator process survives Ctrl+C in the parent terminal. Merged from main.

BootEmulatorAsync

Internally updated to consume the new LaunchEmulator return — no change to its public signature or EmulatorBootResult.

Public API

PublicAPI.Unshipped.txt updated for both net10.0 and netstandard2.0.

Tests

• All fake-emulator Windows batch scripts use timeout /t N /nobreak >nul instead of ping -n N 127.0.0.1 >nul
• Tests track and kill the specific Process returned by LaunchEmulator rather than killing by process name
• New tests: LaunchEmulator_SurvivesSigint, InvalidEmulatorBinary_ReturnsLaunchFailed, ShellQuote_EscapesSingleQuotes, plus unit tests for TryResolvePortsFromLine and ResolveAvdLogPath

[rmarinho]

/review

[Copilot]

Pull request overview

This PR enriches emulator launching in Xamarin.Android.Tools.AndroidSdk by returning an EmulatorLaunchResult (serial/ports/log path) instead of a raw Process, avoiding caller-side races and AOSP path assumptions.

Changes:

• Introduces EmulatorLaunchResult to carry process metadata, ports/serial, log path, and a PortsResolvedAsync signal.
• Updates EmulatorRunner.LaunchEmulator to support pre-assigned ports, log path resolution, and late-bound port parsing from emulator output.
• Adds/updates tests and updates PublicAPI unshipped files for both TFMs.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.

File	Description
src/Xamarin.Android.Tools.AndroidSdk/Runners/EmulatorRunner.cs	Changes LaunchEmulator to return EmulatorLaunchResult, resolve log path, and asynchronously resolve ports from output.
src/Xamarin.Android.Tools.AndroidSdk/Models/EmulatorLaunchResult.cs	Adds the new public result type surfaced by LaunchEmulator.
tests/Xamarin.Android.Tools.AndroidSdk-Tests/EmulatorRunnerTests.cs	Adds tests for pre-assigned ports, output-parsed ports, and log path resolution.
src/Xamarin.Android.Tools.AndroidSdk/PublicAPI/*/PublicAPI.Unshipped.txt	Records the new/changed public API surface for netstandard2.0 and net10.0.

[github-actions]

@copilot 🤖 ⚠️ Needs Changes — 2 warnings, 2 suggestions

What's good:

• EmulatorLaunchResult is a clean, well-documented type that solves a real problem (callers no longer race adb devices or hardcode AOSP log paths)
• TryResolvePortsFromLine is nicely extracted as a testable internal static method
• Thorough test coverage including unit tests for parsing and integration tests with fake emulators
• Good use of TaskCompletionSource with RunContinuationsAsynchronously to avoid inline continuations
• ResolveAvdLogPath correctly respects ANDROID_AVD_HOME / ANDROID_USER_HOME via EnvironmentVariableNames constants
• PublicAPI files updated for both TFMs

Issues:

#	Severity	Category	Summary
1	⚠️	Async pattern	Exited event can race with OutputDataReceived, faulting PortsResolvedAsync even when port lines were emitted
2	⚠️	Error handling	consolePort doc says "even number in 5554–5682" but no validation is performed; adbPort without consolePort silently ignored
3	💡	Pattern	Stderr log level downgraded from Warning to Verbose — may bury important emulator errors
4	💡	Error handling	Bare catch { exitCode = -1; } doesn't log the exception

CI: "Agent" check is still in progress; "Prepare" and "license/cla" passed.

Generated by Android Tools PR Reviewer for issue #340 · ● 3.7M

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[rmarinho]

@copilot can you rebase and fix conflicts

[Copilot]

@copilot can you rebase and fix conflicts

Done — merged main into the branch (commit 2ed3fc3). Conflicts in EmulatorRunner.cs and EmulatorRunnerTests.cs were resolved, incorporating the SIGINT isolation, process.Start() return check, using (launchResult.Process) pattern, and InvalidOperationException guard from main. All 39 tests pass.

[rmarinho]

/review

[github-actions]

✅ Android Tools PR Reviewer completed successfully!

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

[github-actions]

@copilot ⚠️ Needs Changes — 2 warnings, 2 suggestions

Summary

Well-structured change that replaces a bare Process return with a rich EmulatorLaunchResult — the port/serial/log-path bundling eliminates caller boilerplate and the stdout/stderr parsing for late-bound ports is a solid approach. Good test coverage with unit tests for the parsing logic and integration tests for both pre-assigned and late-bound port paths. The cleanup of test helpers (TryKillProcess replacing GetProcessesByName patterns) is a welcome improvement.

Issues

Severity	Category	File	Description
⚠️	Async pattern	EmulatorRunner.cs:190-194	LaunchEmulatorAsync cancellation throws InvalidOperationException instead of OperationCanceledException — breaks the async cancellation contract
⚠️	Pattern	EmulatorRunner.cs:97-100	Null-forgiving operator logFile! used after null check — use property pattern for proper flow analysis
💡	Code organization	EmulatorLaunchResult.cs:34-40	Thread-safety invariants for ConsolePort/AdbPort should be documented
💡	API design	EmulatorRunner.cs:208	Good testability via internal static TryResolvePortsFromLine

Positive callouts

• StartFireAndForget is a clean extraction into ProcessUtils — follows the repo convention of centralizing process management
• ResolveAvdLogPath correctly uses EnvironmentVariableNames constants and respects the runner-level environmentVariables overrides
• TaskCreationOptions.RunContinuationsAsynchronously on the TCS prevents potential deadlocks from inline continuations
• Test improvements: timeout /t N /nobreak replaces ping -n N on Windows, process cleanup is per-handle rather than by-name — both are more robust

Generated by Android Tools PR Reviewer for issue #340 · ● 4M

[github-actions]

🤖 ⚠️ Async pattern — When the caller cancels via cancellationToken, the registration kills the process, which triggers onExited → tcs.TrySetException(new InvalidOperationException(...)). The await then throws InvalidOperationException instead of the expected OperationCanceledException, breaking the standard async cancellation contract.

Callers that catch (OperationCanceledException) (including the existing BootEmulatorAsync) will not handle this correctly — the InvalidOperationException will propagate as an unhandled failure.

Consider wrapping the await to translate cancellation-caused faults:

try {
	await result.PortsResolvedAsync.ConfigureAwait (false);
} catch (InvalidOperationException) when (cancellationToken.IsCancellationRequested) {
	cancellationToken.ThrowIfCancellationRequested ();
}

Rule: CancellationToken contract (Async pattern)

[github-actions]

🤖 ⚠️ Pattern — The null-forgiving operator logFile! is used to silence the nullable warning after the IsNullOrWhiteSpace check, but the repo convention bans postfix ! after null checks. string.IsNullOrWhiteSpace doesn't provide the compiler with flow-analysis narrowing, so the workaround is a property pattern:

if (logFile is { } lf && !string.IsNullOrWhiteSpace (lf)) {
	resolvedLogPath = lf;
	args.Add ("-logfile");
	args.Add (lf);
}

This gives proper non-null flow analysis on all target frameworks. Same applies on line 100.

Rule: No null-forgiving operator

[github-actions]

🤖 💡 Code organization — ConsolePort and AdbPort are mutated from TryResolvePortsFromLine, which runs on process DataReceived event handler threads (stdout/stderr events can fire from different thread pool threads). The class doesn't document its thread-safety assumptions.

Per repo convention, consider adding a <remarks> on the class:

/// <remarks>
/// <see cref="ConsolePort"/> and <see cref="AdbPort"/> are written by process
/// output callbacks and should only be read after <see cref="PortsResolvedAsync"/> completes.
/// </remarks>

This makes the ordering contract explicit for future callers.

Rule: Document thread-safety invariants

[github-actions]

🤖 💡 API design — Clean design choice making TryResolvePortsFromLine a testable internal static method. The unit tests (TryResolvePortsFromLine_ConsolePort_Parsed, _BothPorts_CompletesTask, _UnrelatedLine_NoEffect) provide good coverage of the parsing logic independent of process management.

Rule: Testability