Repository navigation
feat: list iPhones and iPads over usbmuxd + lockdown - #78
Conversation
getPhysicalDevices asks usbmuxd (ListDevices) and each device's lockdownd (GetValue, no pairing session) instead of spawning `devicectl list devices`, which takes a second or more. devicectl stays as the fallback when usbmuxd or a device does not answer. Device ids are hardware UDIDs on both paths (devicectl's hardwareProperties.udid instead of the CoreDevice identifier), so they do not change between the two. devicectl --device accepts UDIDs, so install / launch are unaffected. The usbmuxd framing moves from apple_device_watch.dart into usbmux.dart and is shared with the watch. The desktop app reads the device model for its frame from lockdown too, keeping devicectl as the fallback.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 12
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| try { | ||
| final devices = <Device>[]; | ||
| for (final props in await _attachedDevices(socketPath).timeout(timeout)) { | ||
| final values = await _lockdownValues( |
There was a problem hiding this comment.
[WARNING]: One unresponsive device discards every device listed
_lockdownValues throws for a device that does not answer (a timeout, or a Connect that usbmuxd refuses), and the catch at line 149 turns that into null for the whole list: the devices already read are dropped and the caller falls back to the slow devicectl path. The same happens for the props['DeviceID']! null-check on the next line. Catch per device and skip the ones that fail, so one bad device does not cost the whole listing.
| final values = await _lockdownValues( | ||
| socketPath, | ||
| props['DeviceID']! as int, | ||
| ).timeout(timeout); |
There was a problem hiding this comment.
[WARNING]: The timeout never cancels the pending socket work
Future.timeout does not cancel the future it wraps: when it fires, _lockdownValues (and _attachedDevices on line 141) is still blocked in reader.next(), so the finally that calls socket.destroy() never runs and the unix socket is leaked for the life of the process - exactly in the case the timeout exists for (usbmuxd accepts the connection but never answers). The timeout is also applied per step, so one listing can take timeout * (N + 1) (5 s per device) and outlast the caller own budget: the app gives up after 20 s and then shows no iOS devices at all, without ever reaching the devicectl fallback. Destroy the socket when the timeout fires, and give the whole usbmuxd phase a single deadline.
| for (final props in await _attachedDevices(socketPath).timeout(timeout)) { | ||
| final values = await _lockdownValues( | ||
| socketPath, | ||
| props['DeviceID']! as int, |
There was a problem hiding this comment.
[WARNING]: A missing DeviceID throws and loses the whole list
props['DeviceID']! as int is a null-check plus a cast on data that comes from the daemon. The authoritative DeviceID is the one on the DeviceList entry, but _attachedDevices throws that away and keeps only Properties. If the entry has no integer DeviceID, this throws and (through the catch at line 149) the entire result becomes null, so every device is re-listed through the slow devicectl path. Read the entry-level DeviceID in _attachedDevices and skip entries without one.
| Future<Map<String, Object?>> next() async { | ||
| final header = await read(16); | ||
| final length = ByteData.sublistView(header).getUint32(0, Endian.little); | ||
| return decodePlist(utf8.decode(await read(length - 16))); |
There was a problem hiding this comment.
[SUGGESTION]: The frame length is not validated before the body is read
read(length - 16) assumes length >= 16; UsbmuxDecoder.add checks that on line 106, but next() does not, so a corrupt frame makes read() slice with a negative length (RangeError) and an oversized length makes it wait for bytes that never arrive. The same applies to the lockdownd length prefix on line 247. Validate the length before reading the body.
| ) { | ||
| final version = values['ProductVersion'] as String?; | ||
| return Device.ios( | ||
| id: values['UniqueDeviceID'] as String? ?? '${properties['SerialNumber']}', |
There was a problem hiding this comment.
[SUGGESTION]: The id fallback does not match the id the other path produces
UniqueDeviceID is normally present, but when it is not the id falls back to usbmuxd SerialNumber, which is the UDID without dashes (and the literal string null when SerialNumber is missing as well). The devicectl path (parsePhysicalDevices) produces the dashed hardwareProperties.udid, so the same phone can get two different ids depending on which path listed it, and the app keys its capture session by that id. Normalize the fallback to the dashed UDID.
| 'real' => double.tryParse(e.innerText.trim()), | ||
| 'true' => true, | ||
| 'false' => false, | ||
| 'data' => base64Decode(e.innerText.replaceAll(RegExp(r'\s'), '')), |
There was a problem hiding this comment.
[SUGGESTION]: A malformed data value throws out of the decoder
base64Decode throws a FormatException on invalid base64, but UsbmuxDecoder.add only catches XmlException (line 114), so the exception escapes into the watch loop (apple_device_watch.dart:52) and tears the attach/detach connection down, or aborts a listing in _UsbmuxReader.next. Decode data defensively so one bad frame is skipped instead of killing the stream.
| ); | ||
|
|
||
| test('uses usbmuxd without running devicectl', () async { | ||
| if (!Platform.isMacOS) return; |
There was a problem hiding this comment.
[WARNING]: The new getPhysicalDevices tests never assert anything in CI
Both new tests return early unless Platform.isMacOS, and CI runs the workspace tests on ubuntu-latest (the macOS job in .github/workflows/ci.yaml only builds the binary), so the behaviour this PR adds - usbmuxd first, devicectl only as a fallback - is never verified. IOSDeviceService already takes pathExists and listUsbmuxDevices overrides for tests; take an isMacOS flag the same way so the guard can be bypassed.
| /// Hardware model (`iPhone15,2`) from lockdownd, or devicectl when | ||
| /// usbmuxd does not have the device. | ||
| Future<String?> _productType() async { | ||
| final values = await lockdownValues(udid); |
There was a problem hiding this comment.
[SUGGESTION]: The model lookup opens a second usbmuxd session
lockdownValues(udid) runs a fresh ListDevices + Connect + GetValue (5 s timeout each) on every stream start, although IOSDeviceService.getPhysicalDevices already read the same lockdownd values and dropped ProductType from the Device. When the id is not a UDID (the devicectl path can produce a CoreDevice UUID, which lockdownValues never matches) the lookup is a wasted round trip, and in the fallback case it delays the device frame by up to 10 s before devicectl even starts. Carry the model on Device or reuse the values from the listing.
Code Review SummaryStatus: 8 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (12 files)
Fix these issues in Kilo Cloud Reviewed by free · Input: 153.5K · Output: 34.8K · Cached: 1.2M |
Description
Physical Apple devices are listed without spawning
devicectl(a second or more per refresh):simutil_apple/lib/src/usbmux.dart: asks usbmuxd (ListDevices), then each device's lockdownd over a usbmuxdConnectto port 62078 (GetValue, no pairing session) forDeviceName,ProductVersion,ProductType,UniqueDeviceID. Pure Dart sockets + XML plist (xmlpackage, already in the lock file). A device on both USB and Wi-Fi is listed once, preferring USB. 5 s timeout per step.IOSDeviceService.getPhysicalDevices: usbmuxd first, falling back todevicectl list devicesonly when usbmuxd or a device does not answer (null). The usbmux list only has hardware, so there are no simulators to filter out.hardwareProperties.udidover the CoreDeviceidentifier.devicectl --deviceaccepts UDIDs, so install / launch are unaffected.apple_device_watch.dartand is shared with the attach/detach watch.IosDeviceSessionreads the model for the device frame from lockdown (lockdownValues), with devicectl as the fallback.Known gap: a device reachable only over a CoreDevice Wi-Fi tunnel (not through usbmuxd) is not listed while usbmuxd answers.
Verified
dart analyze --fatal-infos(simutil_apple, simutil, simutil_shared) andflutter analyze(app) clean; simutil_apple tests pass, including end-to-end tests against a fake usbmuxd on a unix socket.[]in ~110 ms.Type of Change