Add ADB device tracking (host:track-devices) for real-time device monitoring#327
Add ADB device tracking (host:track-devices) for real-time device monitoring#327
Conversation
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>
There was a problem hiding this comment.
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
AdbDeviceTrackerwith reconnect/backoff, snapshot state (CurrentDevices), and callback-driven tracking viaStartAsync. - Adds unit tests validating lifecycle guards and length-prefixed protocol parsing behavior.
- Updates PublicAPI unshipped files for
netstandard2.0andnet10.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. |
| readonly int port; | ||
| readonly Action<TraceLevel, string> logger; | ||
| readonly string? adbPath; | ||
| readonly IDictionary<string, string>? environmentVariables; | ||
| IReadOnlyList<AdbDeviceInfo> currentDevices = Array.Empty<AdbDeviceInfo> (); | ||
| CancellationTokenSource? trackingCts; | ||
| bool disposed; | ||
|
|
||
| /// <summary> | ||
| /// Creates a new AdbDeviceTracker. | ||
| /// </summary> | ||
| /// <param name="adbPath">Optional path to the adb executable for starting the server if needed.</param> | ||
| /// <param name="port">ADB daemon port (default 5037).</param> | ||
| /// <param name="environmentVariables">Optional environment variables for adb processes.</param> | ||
| /// <param name="logger">Optional logger callback.</param> | ||
| public AdbDeviceTracker (string? adbPath = null, int port = 5037, | ||
| IDictionary<string, string>? environmentVariables = null, | ||
| Action<TraceLevel, string>? logger = null) | ||
| { | ||
| if (port <= 0 || port > 65535) | ||
| throw new ArgumentOutOfRangeException (nameof (port), "Port must be between 1 and 65535."); | ||
| this.adbPath = adbPath; | ||
| this.port = port; | ||
| this.environmentVariables = environmentVariables; | ||
| this.logger = logger ?? RunnerDefaults.NullLogger; |
There was a problem hiding this comment.
adbPath and environmentVariables are assigned but never used, which will raise CS0414 (and can fail Release builds when warnings are treated as errors). Either implement the advertised behavior (e.g., start/ensure the ADB server using adbPath and pass environmentVariables to that process) or remove these fields/ctor parameters to avoid a misleading public API surface.
Co-authored-by: Copilot <175728472+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>
…dotnet/android-tools into features/323-adb-device-tracker
| var statusBytes = await ReadExactBytesAsync (s, 4, cancellationToken).ConfigureAwait (false); | ||
| var status = Encoding.ASCII.GetString (statusBytes, 0, 4); |
There was a problem hiding this comment.
Is there an overload where you can pass in a Span<byte> buffer = stackalloc byte[4];
This would prevent every call allocating a byte[] on the heap.
There are several examples in this file:
android-tools/src/Microsoft.Android.Build.BaseTasks/Files.cs
Lines 540 to 542 in ed6aab1
There was a problem hiding this comment.
Good point. Unfortunately stackalloc can't be used across await boundaries (the compiler prohibits it in async methods), and Stream.ReadAsync requires either byte[] or Memory<byte> — neither works with stack-allocated memory.
What I did instead:
SendCommandAsync: combines the 4-byte prefix + command into a singlebyte[]and singleWriteAsynccall (one allocation, one syscall instead of two)ReadStatusAsync: compares raw bytes directly ('O','K','A','Y') — avoids theEncoding.ASCII.GetStringallocation for the common path- The 4-byte reads in
ReadExactBytesOrNullAsyncstill need a heapbyte[4]because of the async constraint, but it's minimal
For truly zero-alloc small reads, we'd need a sync code path or PipeReader (which doesn't multi-target well to netstandard2.0). Happy to explore that as a perf follow-up if this becomes hot.
| var lengthBytes = await ReadExactBytesOrNullAsync (stream, 4, cancellationToken).ConfigureAwait (false); | ||
| if (lengthBytes == null) | ||
| return null; | ||
|
|
||
| var lengthHex = Encoding.ASCII.GetString (lengthBytes, 0, 4); | ||
| if (!int.TryParse (lengthHex, System.Globalization.NumberStyles.HexNumber, null, out var length)) | ||
| throw new FormatException ($"Invalid ADB length prefix: '{lengthHex}'"); |
There was a problem hiding this comment.
See if the AI will go through the whole file allocating as few byte[] as possible, using the techniques in Files.cs.
There was a problem hiding this comment.
Ok did another pass with that
|
/review |
|
✅ Android Tools PR Reviewer completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict:
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:ReadLengthPrefixedStringFromStreamAsyncreimplementsReadLengthPrefixedBytesAsyncparsing 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_GREATERhandling throughout
👍 Positives:
- Good reuse of
AdbRunner.ParseAdbDevicesOutputfor device parsing - Proper
IDisposablewith dispose guard and thread-safe disposal via lock - Correct use of
volatileforcurrentDevicescross-thread visibility CancellationTokenproperly propagated to all downstream async calls- Well-structured
OperationCanceledExceptionhandling withwhenguards - Solid test coverage of the wire protocol parsing and lifecycle edge cases
- Follows the
Action<TraceLevel, string>logger convention withRunnerDefaults.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
| internal static async Task<string?> ReadLengthPrefixedStringFromStreamAsync (Stream stream, CancellationToken cancellationToken) | ||
| { | ||
| var lengthBytes = await ReadExactBytesOrNullAsync (stream, 4, cancellationToken).ConfigureAwait (false); | ||
| if (lengthBytes == null) | ||
| return null; | ||
|
|
||
| var lengthHex = Encoding.ASCII.GetString (lengthBytes, 0, 4); | ||
| if (!int.TryParse (lengthHex, System.Globalization.NumberStyles.HexNumber, null, out var length)) | ||
| throw new FormatException ($"Invalid ADB length prefix: '{lengthHex}'"); | ||
|
|
||
| if (length == 0) | ||
| return string.Empty; | ||
|
|
||
| var payload = await ReadExactBytesAsync (stream, length, cancellationToken).ConfigureAwait (false); | ||
| return Encoding.ASCII.GetString (payload, 0, payload.Length); | ||
| } |
There was a problem hiding this comment.
🤖 ReadLengthPrefixedStringFromStreamAsync reimplements the same length-prefix parsing logic as the instance method ReadLengthPrefixedBytesAsync (lines 113–128). If the wire format changes or a bug is fixed in one, the other must be updated manually.
Consider refactoring so the static method delegates to a shared static core, or have the instance method call the static one:
public async Task<string?> ReadLengthPrefixedStringAsync (CancellationToken cancellationToken = default)
{
return await ReadLengthPrefixedStringFromStreamAsync (GetStream (), cancellationToken).ConfigureAwait (false);
}This eliminates the duplication while keeping the static method available for tests.
Rule: Code duplication (Postmortem #25)
| } catch (OperationCanceledException) { | ||
| break; | ||
| } | ||
| backoffMs = Math.Min (backoffMs * 2, MaxBackoffMs); |
There was a problem hiding this comment.
🤖 backoffMs is only ever increased, never reset after a successful reconnection. If the tracker connects successfully and runs for hours before the connection drops, it will use the accumulated high backoff instead of starting fresh at 500ms.
Add backoffMs = InitialBackoffMs; at the top of TrackDevicesAsync (after the connection succeeds and tracking starts) to reset on each successful connection:
await adb.EnsureOkayAsync (cancellationToken).ConfigureAwait (false);
logger.Invoke (TraceLevel.Verbose, "ADB tracking active");Or simply reset it in the reconnect loop right before calling TrackDevicesAsync:
while (!token.IsCancellationRequested) {
try {
backoffMs = InitialBackoffMs; // reset on each attempt that succeeds
await TrackDevicesAsync (onDevicesChanged, token).ConfigureAwait (false);
}Actually, the reset should happen after a successful connection, not before an attempt. Consider resetting inside TrackDevicesAsync via an out parameter, or restructuring so backoffMs resets when tracking enters the read loop successfully.
Rule: Behavioral bug — backoff state leak across reconnections
…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>
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>
Summary
Add support for real-time device connection/disconnection monitoring via the ADB daemon socket protocol (\host:track-devices-l).
Changes
et10.0\ and
etstandard2.0\
API
\\csharp
public sealed class AdbDeviceTracker : IDisposable
{
public AdbDeviceTracker(string? adbPath = null, int port = 5037, ...);
public IReadOnlyList CurrentDevices { get; }
public Task StartAsync(Action<IReadOnlyList> onDevicesChanged, CancellationToken cancellationToken = default);
public void Dispose();
}
\\
Closes #323