Repository navigation
fix: support foldable simulators lent by an external device provider - #1490
mateuszaliyev wants to merge 2 commits into
Conversation
latekvo
left a comment
There was a problem hiding this comment.
[Diplomat, Opus 5.5]: I ran the review moves on a6c8944: claims vs code, nearest twin (tv-control and the other per-device /tmp sockets), non-happy paths, inputs, reachability and lifetime, plus an absence pass and 9 mutations of the new branches. I also ran list-devices end to end in-process against fake providers, with the provider's device set both configured and not configured, and compared the results with merge-base 91542bd. The foldable flag reaches the external row in both cases. socketTag gives distinct tags for every external id shape I tried. The findings are inline.
| const externalFoldable = await withDeadline( | ||
| externalFoldables(external, ios), | ||
| external.map(() => false), | ||
| "external-foldable" | ||
| ); |
There was a problem hiding this comment.
[Diplomat, Opus 5.5]: This step starts only after the whole Promise.all fan-out has settled, and it has a second BRANCH_DEADLINE_MS backstop of its own. Take a wedged CoreSimulator while a provider lends an iOS simulator. The ios branch times out to [], so listed misses, and each lent simulator gets a second simctl list devices that also runs to its 10 s timeout. A miss is never memoized, so this happens on every call, not just the first.
I reproduced it in-process: an esbuild bundle of listDevicesTool, a fake provider lending 2 simulators, and an xcrun shim that hangs on any simctl call. Over two calls, merge-base 91542bd took 10005 ms and 10005 ms with 2 simctl spawns. This head took 20010 ms and 20006 ms with 6 spawns. The 25 s backstop on the fan-out no longer bounds the tool. list-devices-deadline.test.ts checks single branches only and does not account for this sequential step.
There was a problem hiding this comment.
Good catch, the foldable check now runs inside the external discovery branch. With a hung simctl, the wait is one timeout again. A test now checks that the foldable check does not wait for the plain listing.
| * Whether each external device is a foldable simulator. The plain simctl row a | ||
| * provider's simulator shadows (its device set is in | ||
| * `ios.additionalDeviceSets`) already carries the verdict; otherwise it is read | ||
| * from the provider's device set, memoized per UDID. A foldable only differs |
There was a problem hiding this comment.
[Diplomat, Opus 5.5]: "memoized per UDID" holds only when the listing finds the device, because isFoldableSimulator caches only if (found) (ios-devices.ts:283-285). Take a lent simulator whose declared device set does not list its nativeId: a stale descriptor after the simulator was deleted, or a device simctl reports as isAvailable: false. For each such device, every list-devices call runs one more xcrun simctl --set <set> list devices --json after the fan-out.
I measured this with a fake provider lending 2 simulators from an existing but empty set, over 3 calls. Merge-base: 1001, 474 and 469 ms, with no --set spawns. This head: 1152, 905 and 899 ms, with 6 --set spawns (2 on every call). The PR description says only the first call is slower, which does not hold in this case.
There was a problem hiding this comment.
The lookup now runs in parallel with the other discovery branches, so a miss no longer makes list-devices slower. Misses are kept uncached on purpose, because a device that the set does not list yet should still get the flag once it shows up. This only happens when a provider lists a device that is missing from its own set, which is a provider bug. Updated the PR description.
There was a problem hiding this comment.
[Diplomat, Opus 5.5]: Still present as of 4b62d5c. A lent simulator that its set does not list still spawns simctl --set <set> list devices on every call: 5 calls gave 5 --set spawns. That is one per lent simulator per call, with no dedup when several share a set, so "one simctl call per lent simulator per tool-server run" in the description does not hold for them.
"Adds no time" holds only while another branch is slower. With an empty default set, the external branch is the slowest one:
| /** | ||
| * Present and true for a foldable simulator (the iPhone Duo), as on a plain | ||
| * iOS row. | ||
| */ | ||
| foldable?: true; |
There was a problem hiding this comment.
[Diplomat, Opus 5.5]: An external row can now carry foldable: true, but the "What the agent sees" section of packages/docs/docs/reference/device-providers.mdx still lists only external, provider and capabilities as the extra fields of an external entry. reference/tools.mdx and fundamentals/supported-platforms.mdx describe the flag only for plain simulators. The repo CLAUDE.md asks for the docs update in the same PR, or a line in the description saying none is needed. This PR has neither.
There was a problem hiding this comment.
Added foldable to the "What the agent sees" table.
| * A plain UDID keeps its first 8 characters, the name these sockets have | ||
| * always had. Any other id (an external provider's `ext:<provider>:<udid>`) | ||
| * would collapse to its shared prefix that way, so every device of a provider | ||
| * got one socket; it gets 12 hex characters of its SHA-256 instead, which |
There was a problem hiding this comment.
[Diplomat, Opus 5.5]: This comment describes the change rather than the current code: "the name these sockets have always had" and "would collapse to its shared prefix that way, so every device of a provider got one socket". A reader who never saw the old slice(0, 8) cannot tell what got one socket. The scope is also inaccurate. The old tag of an ext: id was ext: plus the first 4 characters of the provider id, so it was shared by every provider whose id starts with the same 4 characters, not only by the devices of one provider.
There was a problem hiding this comment.
Rewrote the comment to describe the current code, with the correct scope.
|
|
||
| function getSocketPath(udid: string): string { | ||
| return `/tmp/ax-${udid.slice(0, 8)}.sock`; | ||
| return `/tmp/ax-${socketTag(udid)}.sock`; |
There was a problem hiding this comment.
[Diplomat, Opus 5.5]: No test checks that this call site, or getNativeDevtoolsSocketPath (native-devtools.ts:510), uses socketTag. I reverted each one to udid.slice(0, 8) and ran 17 test files (372 tests): socket-tag, list-devices, the ax-service, native-devtools, external-devices/attach and foldable files. All tests pass. socket-tag.test.ts tests only the helper. A refactor that drops the call would bring back the shared /tmp/ax-ext:rado.sock for two lent devices, and CI would stay green.
There was a problem hiding this comment.
Both socket paths now come from small helpers in socket-tag.ts (axServiceSocketPath, nativeDevtoolsSocketPath), the blueprints call them and socket-tag.test.ts tests them.
There was a problem hiding this comment.
[Diplomat, Opus 5.5]: Still present as of 4b62d5c for the ax-service call site. I reverted ax-service.ts:345 to `/tmp/ax-${udid.slice(0, 8)}.sock` and ran the full tool-server suite in 6 shards. Every shard passed, with the same counts as the unmutated head. socket-tag.test.ts calls only the helpers, and every test that reaches the factory uses a plain UDID, where the old and new spellings agree. Reverting the native-devtools call site has no reachable effect, because a claimed ext: device always attaches to the provider's lentSocketPath.
| expect(isFoldableSimulator).toHaveBeenCalledWith(`ext:acme-3f2a9c:${CLAIMED_UDID}`); | ||
| }); | ||
|
|
||
| it("leaves foldable off a claimed simulator that is not foldable", async () => { |
There was a problem hiding this comment.
[Diplomat, Opus 5.5]: This test does not tell main and this PR apart. With origin/main's list-devices.ts in place, it stays green, and only "marks a claimed foldable simulator with foldable: true" fails.
The PR description names the shadowed simctl row as the first source of the flag, but no test covers a foldable shadowed row. I replaced listed.get(device.nativeId) ?? … (list-devices.ts:280) with an unconditional isFoldableSimulator call, and every list-devices test passes. If that path broke, CI would not notice, either for the flag or for the claim that the extra ~0.2 s applies only when the device set is not configured.
There was a problem hiding this comment.
Removed the shadowed-row shortcut, so every lent simulator now goes through the same check. The non-foldable test now asserts that the check ran and a new test covers the timing issue from the other comment.
latekvo
left a comment
There was a problem hiding this comment.
[Diplomat, Opus 5.5]: Second pass on 4b62d5c. I ran the review moves again: claims vs code, nearest twin, non-happy paths, inputs, reachability and lifetime. I also ran 12 mutations of the new branches and constants, and ran both fixes end to end through dist/index.js start, compared with merge-base 91542bd, with a fake provider lending two booted simulators.
- Sockets, two concurrent cold
describecalls. On 91542bd both devices shared/tmp/ax-ext:rado.sock. One device returned an empty tree in 6 of 6 rounds, and the other timed out or lost its ax-service in 5 of 6. On this head the devices got/tmp/ax-125f4ad261dc.sockand/tmp/ax-153a8bcec0fc.sock, and both returned the right screen in 6 of 6 rounds. - Foldable. An
xcrunshim served a device type with two integrated displays. The lent Duo row gotfoldable: truewith and without the set inios.additionalDeviceSets, and the single-panel row did not. - The full tool-server suite (6918 passed), both tsc configs, prettier and eslint are green.
Fixed from the first pass: the docs row, the socket-tag comment, the non-foldable test, and the sequential latency. The uncached miss and the unpinned ax-service call site are still present; I replied on those threads. One new finding is inline. Two issues outside the diff are filed as #1531 and #1532.
| * them; the branch's worst case (a provider probe, then one `simctl` listing | ||
| * and one device-type read) stays under `BRANCH_DEADLINE_MS`. |
There was a problem hiding this comment.
[Diplomat, Opus 5.5]: The worst case is longer than this says. After the probe (800 ms), the chain can run simctl --set <set> list devices (10 s), then simctl list devicetypes (10 s). It then runs the re-read that deviceTypeBundlePath does on a miss (another 10 s; a timed-out listing comes back as an empty map, so it always misses), then plutil (5 s). That is 35.8 s against the 25 s deadline. list-devices-deadline.test.ts pins the Vega and Android sums, but not this branch. When the deadline fires, withDeadline replaces the whole external result with []. That call then lists no lent device from any provider, including lent Android emulators, which never needed the check. On main this branch was the probe alone.
I reproduced it through the HTTP entry point (dist/index.js start, POST /tools/list-devices). An xcrun shim delays every simctl call by 8.5 s, and the lent Duo's type is missing from list devicetypes. The descriptor lends two iOS simulators and emulator-5554.
- 91542bd: call 1 took 9 s and listed all 3
ext:rows. - 4b62d5c: call 1 took 25 s and listed no
ext:rows. The log said[list-devices] external discovery exceeded 25000ms; returning partial results, and call 2 recovered.
An in-process run gave the same result: a 6 s --set listing plus a hanging list devicetypes returned external=[] after 25020 ms, against the row in 30 ms on main. A full wedge, where every simctl call hangs, stops at about 10 s and does not trip the deadline.
This pull request fixes two bugs with iOS simulators lent to Argent by an external device provider, such as Radon IDE (ids shaped
ext:<provider>:<udid>):list-devicesnever setfoldable: trueon a provider's iPhone Duo, so an agent couldn't tell thatfoldapplies. The provider row replaces thesimctlrow that carried the flag. Now Argent reads the flag for each provider simulator from the provider's device set.ext:radofor every provider device. When the ax services of two lent devices started at the same moment, one device silently got the other device's screen and the other got an empty tree. Now a plain UDID keeps its 8-character path and any other id uses the first 12 hex characters of its SHA-256. Thenative-devtoolssocket path uses the same tag for consistency.How has this been tested
With Radon IDE lending an iPhone Duo (iOS 27.1) and an iPhone 17 Pro (iOS 26.5), comparing
mainwith this branch:mainlists the Duo withoutfoldable; this branch lists it withfoldable: true, with and without Radon's device set inios.additionalDeviceSets. The iPhone 17 Pro never gets the flag, andfoldworks on the lent Duo. The foldable check runs alongside the rest of device discovery, so it adds no time tolist-devices. It costs onesimctlcall per lent simulator per tool-server run.main, starting both devices' ax services at once gave the Duo the iPhone's Settings screen in 4 of 4 runs. On this branch each device has its own socket, 3 of 3 concurrent starts returned the right screens and taps landed on the right device./tmp/ax-<first 8 of udid>.sock).list-devices.test.ts,socket-tag.test.ts).