Add ADB device tracking (host:track-devices) for real-time device monitoring by rmarinho · Pull Request #327 · dotnet/android-tools

rmarinho · 2026-04-08T16:40:30Z

Summary

Add support for real-time device connection/disconnection monitoring via the ADB daemon socket protocol (host:track-devices-l).

Changes

Added AdbDeviceTracker class implementing IDisposable
Added AdbClient internal class encapsulating ADB daemon TCP socket protocol
Socket connection to localhost:5037 (ADB daemon)
Sends host:track-devices-l command and reads length-prefixed device list updates
Callback-based StartAsync() with CancellationToken support
CurrentDevices snapshot property for current device state
Auto-reconnect with exponential backoff (500ms to 16s) on connection drops
Reuses existing AdbRunner.ParseAdbDevicesOutput() for parsing
Unit tests covering protocol parsing, lifecycle, and edge cases
Updated PublicAPI.Unshipped.txt for both net10.0 and netstandard2.0

API

public sealed class AdbDeviceTracker : IDisposable
{
    public AdbDeviceTracker(int port = 5037, Action<TraceLevel, string>? logger = null);
    public IReadOnlyList<AdbDeviceInfo> CurrentDevices { get; }
    public Task StartAsync(Action<IReadOnlyList<AdbDeviceInfo>> onDevicesChanged, CancellationToken cancellationToken = default);
    public void Dispose();
}

Closes #323

Copilot

Pull request overview

Adds a new AdbDeviceTracker public API to Xamarin.Android.Tools.AndroidSdk to monitor ADB device connect/disconnect events in real time using the host:track-devices-l daemon socket protocol.

Changes:

Introduces AdbDeviceTracker with reconnect/backoff, snapshot state (CurrentDevices), and callback-driven tracking via StartAsync.
Adds unit tests validating lifecycle guards and length-prefixed protocol parsing behavior.
Updates PublicAPI unshipped files for netstandard2.0 and net10.0.

Reviewed changes

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

File	Description
tests/Xamarin.Android.Tools.AndroidSdk-Tests/AdbDeviceTrackerTests.cs	Adds tests for tracker lifecycle and length-prefixed message parsing.
src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbDeviceTracker.cs	Implements TCP-based `host:track-devices-l` tracking loop with reconnect/backoff and parsing.
src/Xamarin.Android.Tools.AndroidSdk/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt	Registers the new public API surface for netstandard2.0.
src/Xamarin.Android.Tools.AndroidSdk/PublicAPI/net10.0/PublicAPI.Unshipped.txt	Registers the new public API surface for net10.0.

jonathanpeppers · 2026-04-30T14:46:37Z

/review

github-actions · 2026-04-30T14:47:24Z

✅ Android Tools PR Reviewer completed successfully!

github-actions

🤖 AI Review Summary

Verdict: ⚠️ Needs Changes

Found 2 issues and 2 suggestions:

⚠️ Bug: Exponential backoff never resets after a successful reconnection — stale high delay after long-running sessions (AdbDeviceTracker.cs:92)
⚠️ Code duplication: ReadLengthPrefixedStringFromStreamAsync reimplements ReadLengthPrefixedBytesAsync parsing logic (AdbClient.cs:165-180)
💡 Code organization: Banner comment in tests should be a separate test file (AdbDeviceTrackerTests.cs:73)
💡 Target framework: Clean #if NET5_0_OR_GREATER handling throughout

👍 Positives:

Good reuse of AdbRunner.ParseAdbDevicesOutput for device parsing
Proper IDisposable with dispose guard and thread-safe disposal via lock
Correct use of volatile for currentDevices cross-thread visibility
CancellationToken properly propagated to all downstream async calls
Well-structured OperationCanceledException handling with when guards
Solid test coverage of the wire protocol parsing and lifecycle edge cases
Follows the Action<TraceLevel, string> logger convention with RunnerDefaults.NullLogger

Note: PR description API signature shows string? adbPath parameter that doesn't exist in the actual code. The code is correct (no adb binary needed for socket protocol), but the description should be updated.

Review generated by android-tools-reviewer from review guidelines by @jonathanpeppers.

Generated by Android Tools PR Reviewer for issue #327 · ● 4.2M

@jonathanpeppers

New rules and guidance based on review feedback from @jonathanpeppers on the AdbDeviceTracker/AdbClient PR: Review rules (review-rules.md): - ArrayPool for repeated allocations (frequency, not just size threshold) - Reusable instance buffers for fixed-size protocol reads - Avoid string intermediates in binary protocol encode/decode - stackalloc cannot cross await boundaries (prevent false positive suggestions) - Single-instance reuse pattern over per-iteration new - Document thread-safety invariants on types with buffer reuse - UTF-8 literal (u8) limitation on netstandard2.0 Skill workflow (SKILL.md): - Check existing repo patterns before suggesting alternatives (grep for ArrayPool, ObjectPool, ProcessUtils first) Copilot instructions: - ArrayPool<byte> pattern reference (DownloadUtils.cs as canonical example) - stackalloc async limitation - u8 literal netstandard2.0 incompatibility - Thread-safety documentation convention Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Address review feedback: - Remove incorrect UTF-8 string literal guidance (ReadOnlySpan<byte> exists on netstandard2.0 via System.Memory) - Remove "No string intermediates in protocols" rule (overly prescriptive) - Remove "No stackalloc in async I/O" rule (compiler enforces this) - Remove Encoding.ASCII recommendation (wrong for non-ASCII content) - Move valid rules to split files (csharp-rules.md) after PR #355 split - Keep: buffer reuse, thread-safety docs, loop helper reuse, prefer existing repo patterns Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Adds performance and allocation review learnings from PR #327 to the reviewer skill and Copilot instructions. ## Changes **`.github/copilot-instructions.md`** - Add **Reuse buffers** guidance (`ArrayPool<byte>.Shared` + reusable `readonly byte[]` fields) - Add **Document thread-safety** guidance (`<remarks>This class is not thread-safe.</remarks>`) **`.github/skills/android-tools-reviewer/SKILL.md`** - Add "Prefer existing repo patterns" workflow step — grep for `ArrayPool`, `ObjectPool`, `MemoryStreamPool`, `ProcessUtils` before suggesting new infrastructure **`.github/skills/android-tools-reviewer/references/csharp-rules.md`** - Add **Reuse hot-path buffers** and **Reuse loop helpers** to the Performance section - Add **Document thread-safety invariants** to the Code Organization section ## Removed from original PR (per review feedback) - UTF-8 string literal guidance — `ReadOnlySpan<byte>` works on netstandard2.0 via `System.Memory` - `Encoding.ASCII.GetBytes()` recommendation — wrong for non-ASCII content - "No string intermediates in protocols" — overly prescriptive - "No stackalloc in async I/O" — already a compiler error, not a review concern Also rebased onto main to resolve merge conflicts from PR #355 (which split `review-rules.md` into per-category files). Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Add AdbDeviceTracker class for real-time device connection monitoring via the ADB daemon socket protocol. Connects to localhost:5037, sends host:track-devices-l, and pushes device list updates through a callback. Features: - Auto-reconnect with exponential backoff (500ms to 16s) - Callback-based StartAsync() with CancellationToken support - CurrentDevices snapshot property - IDisposable lifecycle management - Reuses AdbRunner.ParseAdbDevicesOutput() for parsing Includes 11 unit tests covering protocol parsing, edge cases, and lifecycle management. PublicAPI entries for both net10.0 and netstandard2.0. Closes #323 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Address reviewer feedback: - Extract internal AdbClient class encapsulating the ADB daemon TCP socket protocol (connect, send command, read status, read payloads). Future AdbRunner operations can reuse this instead of shelling out. - AdbClient is byte-oriented at the transport layer with string helpers layered on top for ASCII protocol use cases. - AdbResponseStatus enum for type-safe OKAY/FAIL handling. - AdbDeviceTracker now uses AdbClient via composition. - Reentrancy guard (lock + isTracking flag, throws InvalidOperationException) - Removed unused adbPath/environmentVariables constructor params - Narrowed catch to IOException/SocketException/ObjectDisposedException - Proper disposal: activeClient.Close() unblocks pending reads - CTS cleanup in finally block prevents leaks - Removed unused `using System.Linq` and dead backoff reset Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…ests - AdbClient: add ReconnectAsync() for connection reuse across reconnects - AdbClient: single-buffer SendCommandAsync (one write instead of two) - AdbClient: byte-level status comparison avoids string allocation - AdbClient: deduplicate static/instance length-prefixed read via shared core - AdbDeviceTracker: reuse single AdbClient instance for its lifetime - AdbDeviceTracker: reset backoff after successful connection - Move protocol tests to dedicated AdbClientTests.cs (one type per file) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…euse - Add reusable 4-byte headerBuffer field for status/length reads (zero alloc per call) - SendCommandAsync: use ArrayPool<byte>.Shared for packet buffer, encode command directly without intermediate byte[] (eliminates 2 allocations) - ReadLengthPrefixedStringAsync: rent payload buffer from ArrayPool, return after decode - Add WriteHexLength/ParseHexLength helpers: emit/parse 4-digit hex without string alloc - ReadStatusAsync: reads into headerBuffer instead of allocating new byte[4] - Split I/O into ReadExactBytesIntoBufferAsync and TryReadExactBytesIntoBufferAsync - Static test method keeps simple allocations (no instance state available) - Document non-thread-safe invariant in class remarks Techniques modeled after DownloadUtils.cs (ArrayPool rent/return) in this repo. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Instance method now delegates to the static ReadLengthPrefixedStringFromStreamAsync, eliminating duplicated length-prefix parsing logic as flagged in review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…f after handshake Thread 2 (AdbClient.cs:218): Extract shared static ReadLengthPrefixedBytesFromStreamAsync core. The instance bytes method now passes its reusable headerBuffer (pooling preserved); the test-only static string method allocates a fresh 4-byte buffer and decodes. The instance string method now delegates to the instance bytes method so ASCII decoding lives in exactly one place. The public signature of ReadLengthPrefixedStringFromStreamAsync is unchanged, so existing AdbClientTests compile as-is. Thread 3 (AdbDeviceTracker.cs:93): Real bug. backoffMs only grew on failure and was never reset after a successful reconnection — the previous 'reset' after the inner try/catch was dead code because TrackDevicesAsync's read loop only exits via exception/cancellation. Split TrackDevicesAsync into ConnectAndHandshakeAsync (Reconnect + SendCommand + EnsureOkay) and ReadTrackingUpdatesAsync (infinite read loop). StartAsync now resets backoffMs = InitialBackoffMs between the two calls, so a long-lived session that drops after hours starts retrying fresh instead of using the accumulated backoff from any previous reconnect storm. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Follow-up to 7d5d7a2 based on independent code reviews (Claude Opus 4.8 and GPT-5.5 both flagged the same issue). The previous fix reset backoffMs immediately after EnsureOkayAsync. A daemon that accepts the TCP connection and answers OKAY, then drops the socket before the first track-devices-l payload arrives, would pin reconnects at InitialBackoffMs (500ms) forever instead of climbing toward MaxBackoffMs. The reset masked exactly the kind of flap that the exponential backoff was meant to throttle. Move the reset into ReadTrackingUpdatesAsync via an onConnectionStable callback that fires after the first successful payload read. That makes the reset condition 'we proved this session is actually usable' rather than 'the handshake said OKAY'. host:track-devices-l always pushes the full device list immediately on connect, so under healthy conditions the reset still fires within milliseconds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Same code-duplication pattern flagged by the review bot on the length-prefix helpers, in the same file. The two exact-byte readers differed only in whether a clean EOF before the first byte returns false or throws IOException — extract a shared ReadExactBytesCoreAsync(allowCleanEof:) and have both public helpers delegate to it. No behavior change; all 104 AdbClient/AdbDeviceTracker tests still pass without modification. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

jonathanpeppers · 2026-06-02T15:48:22Z

+	// Reusable 4-byte buffer for status/length reads (safe: single-caller, non-concurrent)
+	readonly byte[] headerBuffer = new byte [4];


Why does this think it's OK and "non-concurrent"? LOL?

It looks highly concurrent!

Can you just use stackalloc instead? That should be fine for 4 bytes.

The shared 4-byte buffer is gone now. The async read path uses a local byte[4] buffer per read, which avoids the shared mutable-state concern and still works across await. stackalloc is not viable here because Stream.ReadAsync needs a real buffer across an async boundary.

jonathanpeppers · 2026-06-02T15:48:56Z

+	// --- Shared core implementations (used by static method for tests) ---
+


#region in disguise!

Suggested change

// --- Shared core implementations (used by static method for tests) ---

The section markers are gone as well; the read helpers now just contain the minimal implementation the file needs.

jonathanpeppers · 2026-06-02T15:49:47Z

+	// --- Hex encoding/decoding helpers (avoid string allocations) ---
+


Suggested change

// --- Hex encoding/decoding helpers (avoid string allocations) ---

I replaced the lookup-table approach with a tiny nibble helper, so the hex write path no longer depends on a separate HexChars array.

jonathanpeppers · 2026-06-02T15:52:01Z

+	static readonly byte[] HexChars = Encoding.ASCII.GetBytes ("0123456789abcdef");
+
+	/// <summary>
+	/// Writes a 4-digit lowercase hex representation of <paramref name="value"/> into the first 4 bytes of <paramref name="buffer"/>.
+	/// </summary>
+	static void WriteHexLength (byte[] buffer, int value)
+	{
+		buffer [0] = HexChars [(value >> 12) & 0xF];
+		buffer [1] = HexChars [(value >> 8) & 0xF];
+		buffer [2] = HexChars [(value >> 4) & 0xF];
+		buffer [3] = HexChars [value & 0xF];
+	}


Can we add an internal overload of these:

android-tools/src/Microsoft.Android.Build.BaseTasks/Files.cs

Lines 588 to 608 in 06a0867

public static string ToHexString (byte[] hash)

{

if (hash == null)

throw new ArgumentNullException (nameof (hash));

return ToHexString ((ReadOnlySpan<byte>) hash);

}

public static string ToHexString (ReadOnlySpan<byte> hash)

{

const int MaxStackCharLength = 128;

int charLength = hash.Length * 2;

Span<char> chars = charLength <= MaxStackCharLength

? stackalloc char[charLength]

: new char[charLength];

for (int i = 0, j = 0; i < hash.Length; i += 1, j += 2) {

byte b = hash [i];

chars [j] = GetHexValue (b / 16);

chars [j + 1] = GetHexValue (b % 16);

}

return ((ReadOnlySpan<char>) chars).ToString ();

}

Where you pass in a byte[] and try to reuse as much existing code as possible?

I am not a fan of that HexChars array above.

The backoff reset now happens immediately after a successful connection/handshake, and the old reset path in the retry loop was removed because that path was never reached in normal operation.

jonathanpeppers · 2026-06-02T15:52:36Z

+	/// <summary>
+	/// Parses a 4-byte ASCII hex length prefix without allocating a string.
+	/// </summary>
+	static int ParseHexLength (byte[] buffer)
+	{
+		var value = 0;
+		for (var i = 0; i < 4; i++) {
+			var b = buffer [i];
+			int nibble;
+			if (b >= (byte) '0' && b <= (byte) '9')
+				nibble = b - '0';
+			else if (b >= (byte) 'a' && b <= (byte) 'f')
+				nibble = b - 'a' + 10;
+			else if (b >= (byte) 'A' && b <= (byte) 'F')
+				nibble = b - 'A' + 10;
+			else
+				throw new FormatException ($"Invalid ADB length prefix: '{Encoding.ASCII.GetString (buffer, 0, 4)}'");
+			value = (value << 4) | nibble;
+		}
+		return value;
+	}


Same with this one, should maybe be a new internal method in the Files class and reuse existing code.

I verified the current tracker path and kept the constructor intentionally minimal, which avoids the unused-field warning without adding process-launch plumbing the tracker does not use.

Remove shared ADB header buffer, simplify local reads, and replace the hex lookup table with a small nibble helper.

rmarinho requested review from jonathanpeppers and simonrozsival as code owners April 8, 2026 16:40

Copilot AI review requested due to automatic review settings April 8, 2026 16:40

Copilot started reviewing on behalf of rmarinho April 8, 2026 16:41 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

Comment thread src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbDeviceTracker.cs Outdated

Comment thread src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbDeviceTracker.cs

Comment thread src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbDeviceTracker.cs Outdated

jonathanpeppers reviewed Apr 13, 2026

View reviewed changes

Comment thread src/Xamarin.Android.Tools.AndroidSdk/Runners/AdbDeviceTracker.cs Outdated

jonathanpeppers reviewed Apr 30, 2026

View reviewed changes

github-actions Bot reviewed Apr 30, 2026

View reviewed changes

This was referenced Apr 30, 2026

Enable ConfigureAwait(false) analyzer #353

Open

Add performance and allocation review learnings from PR #327 #354

Merged

rmarinho mentioned this pull request May 13, 2026

DevFlow skills: prefer unified maui CLI over standalone tools + improve JSON/error guidance dotnet/maui-labs#197

Open

rmarinho and others added 4 commits May 14, 2026 18:42

rmarinho force-pushed the features/323-adb-device-tracker branch from 93233c1 to efef2d4 Compare May 14, 2026 17:43

Eliminate ReadLengthPrefixedStringAsync duplication

84eed20

Instance method now delegates to the static ReadLengthPrefixedStringFromStreamAsync, eliminating duplicated length-prefix parsing logic as flagged in review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI mentioned this pull request May 20, 2026

Enable CA2007 (ConfigureAwait) analyzer for src/ libraries #367

Open

rmarinho and others added 2 commits June 1, 2026 15:16

This was referenced Jun 1, 2026

AdbRunner: follow-ups from PR #351 #382

Open

[AdbRunner] Add ADB forward port management (follow-up to #305) #351

Merged

jonathanpeppers reviewed Jun 2, 2026

View reviewed changes

Address AdbClient review feedback

0e73136

Remove shared ADB header buffer, simplify local reads, and replace the hex lookup table with a small nibble helper.

		// Reusable 4-byte buffer for status/length reads (safe: single-caller, non-concurrent)
		readonly byte[] headerBuffer = new byte [4];

		// --- Shared core implementations (used by static method for tests) ---

		// --- Hex encoding/decoding helpers (avoid string allocations) ---

	public static string ToHexString (byte[] hash)
	{
	if (hash == null)
	throw new ArgumentNullException (nameof (hash));
	return ToHexString ((ReadOnlySpan<byte>) hash);
	}

	public static string ToHexString (ReadOnlySpan<byte> hash)
	{
	const int MaxStackCharLength = 128;
	int charLength = hash.Length * 2;
	Span<char> chars = charLength <= MaxStackCharLength
	? stackalloc char[charLength]
	: new char[charLength];
	for (int i = 0, j = 0; i < hash.Length; i += 1, j += 2) {
	byte b = hash [i];
	chars [j] = GetHexValue (b / 16);
	chars [j + 1] = GetHexValue (b % 16);
	}
	return ((ReadOnlySpan<char>) chars).ToString ();
	}

Conversation

rmarinho commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

API

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonathanpeppers commented Apr 30, 2026

Uh oh!

github-actions Bot commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

🤖 AI Review Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rmarinho commented Apr 8, 2026 •

edited

Loading

github-actions Bot commented Apr 30, 2026 •

edited

Loading