Repository navigation
fix: support foldable simulators lent by an external device provider #1490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ import { | |
| ADB_DEVICES_TIMEOUT_MS, | ||
| } from "../../utils/adb"; | ||
| import { listRunningVvdConsolePorts } from "../../utils/vega-process"; | ||
| import { listIosSimulators, type IosSimulator } from "../../utils/ios-devices"; | ||
| import { isFoldableSimulator, listIosSimulators, type IosSimulator } from "../../utils/ios-devices"; | ||
| import { listIosPhysicalDevices } from "../../utils/ios-device/devicectl"; | ||
| import { simctlListDevices } from "../../utils/sim-remote"; | ||
| import { withRemotePrefix } from "../../utils/device-info"; | ||
|
|
@@ -72,6 +72,11 @@ type AndroidDevice = { | |
| type ExternalListedDevice = { | ||
| capabilities: string[]; | ||
| external: true; | ||
| /** | ||
| * Present and true for a foldable simulator (the iPhone Duo), as on a plain | ||
| * iOS row. | ||
| */ | ||
| foldable?: true; | ||
|
Comment on lines
+75
to
+79
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat, Opus 5.5]: An external row can now carry
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
| id: string; | ||
| kind: "device" | "emulator" | "simulator"; | ||
| name: string; | ||
|
|
@@ -233,10 +238,11 @@ export async function withDeadline<T>(p: Promise<T>, fallback: T, label: string) | |
| } | ||
| } | ||
|
|
||
| function toListedExternal(device: ExternalDevice): ExternalListedDevice { | ||
| function toListedExternal(device: ExternalDevice, foldable: boolean): ExternalListedDevice { | ||
| return { | ||
| capabilities: Array.from(device.capabilities).sort(), | ||
| external: true, | ||
| ...(foldable ? { foldable: true as const } : {}), | ||
| id: device.id, | ||
| kind: device.kind, | ||
| name: device.name, | ||
|
|
@@ -252,6 +258,31 @@ function toListedExternal(device: ExternalDevice): ExternalListedDevice { | |
| }; | ||
| } | ||
|
|
||
| /** | ||
| * 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat, Opus 5.5]: "memoized per UDID" holds only when the listing finds the device, because 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The lookup now runs in parallel with the other discovery branches, so a miss no longer makes
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat, Opus 5.5]: Still present as of 4b62d5c. A lent simulator that its set does not list still spawns "Adds no time" holds only while another branch is slower. With an empty default set, the external branch is the slowest one: |
||
| * in which panel shows the UI, so an agent needs the flag to know `fold` | ||
| * applies, whoever drives the device. | ||
| */ | ||
| async function externalFoldables( | ||
| external: readonly ExternalDevice[], | ||
| ios: readonly IosSimulator[] | ||
| ): Promise<boolean[]> { | ||
| const listed = new Map(ios.map((sim) => [sim.udid, sim.foldable === true])); | ||
|
|
||
| return Promise.all( | ||
| external.map(async (device) => { | ||
| if (device.platform !== "ios" || device.kind !== "simulator") return false; | ||
|
|
||
| return ( | ||
| listed.get(device.nativeId) ?? (await isFoldableSimulator(device.id).catch(() => false)) | ||
| ); | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Native ids a provider is currently claiming. | ||
| * | ||
|
|
@@ -343,6 +374,12 @@ Booted/ready devices are listed first. Platforms whose CLI is unavailable are si | |
|
|
||
| const externalShadows = externalShadowIds(external); | ||
|
|
||
| const externalFoldable = await withDeadline( | ||
| externalFoldables(external, ios), | ||
| external.map(() => false), | ||
| "external-foldable" | ||
| ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat, Opus 5.5]: This step starts only after the whole I reproduced it in-process: an esbuild bundle of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| const iosTagged: IosDevice[] = ios | ||
| .filter((simulator) => !externalShadows.has(simulator.udid)) | ||
| .map((simulator) => ({ platform: "ios", ...simulator })); | ||
|
|
@@ -389,7 +426,7 @@ Booted/ready devices are listed first. Platforms whose CLI is unavailable are si | |
| ...androidDeduped, | ||
| ...chromium, | ||
| ...vega, | ||
| ...external.map(toListedExternal), | ||
| ...external.map((device, i) => toListedExternal(device, externalFoldable[i] === true)), | ||
| ]; | ||
| devices.sort((a, b) => readinessRank(a) - readinessRank(b)); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| import { createHash } from "node:crypto"; | ||
|
|
||
| /** | ||
| * The per-device part of a `/tmp/<prefix>-<tag>.sock` path. | ||
| * | ||
| * 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rewrote the comment to describe the current code, with the correct scope. |
||
| * keeps the path short (macOS caps `sun_path` at 104 bytes) and can never | ||
| * equal an 8-character UDID tag. | ||
| */ | ||
| export function socketTag(deviceId: string): string { | ||
| if (/^[A-Za-z0-9-]+$/.test(deviceId)) return deviceId.slice(0, 8); | ||
| return createHash("sha256").update(deviceId).digest("hex").slice(0, 12); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,13 +97,24 @@ vi.mock("../src/utils/ios-device/devicectl", async () => { | |
| }; | ||
| }); | ||
|
|
||
| // Whether a provider's simulator is foldable is read from its device set when | ||
| // the plain listing does not carry it; that read is the seam, off by default. | ||
| vi.mock("../src/utils/ios-devices", async () => { | ||
| const actual = await vi.importActual<typeof import("../src/utils/ios-devices")>( | ||
| "../src/utils/ios-devices" | ||
| ); | ||
|
|
||
| return { ...actual, isFoldableSimulator: vi.fn(async () => false) }; | ||
| }); | ||
|
|
||
| import * as fs from "node:fs"; | ||
| import * as http from "node:http"; | ||
| import * as os from "node:os"; | ||
| import * as path from "node:path"; | ||
| import type { AddressInfo } from "node:net"; | ||
| import { listDevicesTool } from "../src/tools/devices/list-devices"; | ||
| import { listIosPhysicalDevices } from "../src/utils/ios-device/devicectl"; | ||
| import { isFoldableSimulator } from "../src/utils/ios-devices"; | ||
| import { | ||
| __resetExternalDeviceCacheForTesting, | ||
| __resetProviderWarningsForTesting, | ||
|
|
@@ -703,6 +714,54 @@ describe("list-devices — external provider shadows", () => { | |
| }); | ||
| }); | ||
|
|
||
| it("marks a claimed foldable simulator with foldable: true", async () => { | ||
| /** | ||
| * The provider's device set is not configured, so the plain listing does | ||
| * not see the device and the verdict is read from the provider's set. | ||
| */ | ||
| execFileMock.mockImplementation((cmd: string, args: string[]) => { | ||
| if (cmd === "adb" && args[0] === "devices") { | ||
| return { stderr: "", stdout: "List of devices attached\n" }; | ||
| } | ||
|
|
||
| return { stderr: "", stdout: "" }; | ||
| }); | ||
|
|
||
| vi.mocked(isFoldableSimulator).mockResolvedValueOnce(true); | ||
|
|
||
| const result = await listDevicesTool.execute!({}, {}); | ||
|
|
||
| const claimed = result.devices.find( | ||
| (device) => "udid" in device && device.udid === `ext:acme-3f2a9c:${CLAIMED_UDID}` | ||
| ); | ||
|
|
||
| expect(claimed).toMatchObject({ external: true, foldable: true }); | ||
| expect(isFoldableSimulator).toHaveBeenCalledWith(`ext:acme-3f2a9c:${CLAIMED_UDID}`); | ||
| }); | ||
|
|
||
| it("leaves foldable off a claimed simulator that is not foldable", async () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat, Opus 5.5]: This test does not tell main and this PR apart. With origin/main's 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| execFileMock.mockImplementation((cmd: string, args: string[]) => { | ||
| if (cmd === "xcrun" && args[0] === "simctl") { | ||
| return { stderr: "", stdout: simctlJson() }; | ||
| } | ||
|
|
||
| if (cmd === "adb" && args[0] === "devices") { | ||
| return { stderr: "", stdout: "List of devices attached\n" }; | ||
| } | ||
|
|
||
| return { stderr: "", stdout: "" }; | ||
| }); | ||
|
|
||
| const result = await listDevicesTool.execute!({}, {}); | ||
|
|
||
| const claimed = result.devices.find( | ||
| (device) => "udid" in device && device.udid === `ext:acme-3f2a9c:${CLAIMED_UDID}` | ||
| ); | ||
|
|
||
| expect(claimed).toBeDefined(); | ||
| expect(claimed).not.toHaveProperty("foldable"); | ||
| }); | ||
|
|
||
| it("leaves an unclaimed simulator in the plain listing", async () => { | ||
| execFileMock.mockImplementation((cmd: string, args: string[]) => { | ||
| if (cmd === "xcrun" && args[0] === "simctl") { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| import { describe, it, expect } from "vitest"; | ||
| import { socketTag } from "../src/utils/socket-tag"; | ||
|
|
||
| describe("socketTag", () => { | ||
| it("keeps the first 8 characters of a plain UDID, the existing socket name", () => { | ||
| expect(socketTag("0A1B2C3D-1111-2222-3333-444444444444")).toBe("0A1B2C3D"); | ||
| }); | ||
|
|
||
| it("gives two devices of one external provider distinct tags", () => { | ||
| const a = socketTag("ext:radon-1a2b3c:0A1B2C3D-1111-2222-3333-444444444444"); | ||
| const b = socketTag("ext:radon-1a2b3c:9F8E7D6C-1111-2222-3333-444444444444"); | ||
| expect(a).not.toBe(b); | ||
| expect(a).not.toBe("ext:rado"); | ||
| }); | ||
|
|
||
| it("is stable, short, and filename-safe for a non-UDID id", () => { | ||
| const id = "ext:radon-1a2b3c:0A1B2C3D-1111-2222-3333-444444444444"; | ||
| expect(socketTag(id)).toBe(socketTag(id)); | ||
| expect(socketTag(id)).toMatch(/^[0-9a-f]{12}$/); | ||
| }); | ||
|
|
||
| it("cannot collide with a UDID tag, which is 8 characters long", () => { | ||
| expect(socketTag("ext:x:0A1B2C3D")).toHaveLength(12); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Diplomat, Opus 5.5]: No test checks that this call site, or
getNativeDevtoolsSocketPath(native-devtools.ts:510), usessocketTag. I reverted each one toudid.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.tstests only the helper. A refactor that drops the call would bring back the shared/tmp/ax-ext:rado.sockfor two lent devices, and CI would stay green.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both socket paths now come from small helpers in
socket-tag.ts(axServiceSocketPath,nativeDevtoolsSocketPath), the blueprints call them andsocket-tag.test.tstests them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Diplomat, Opus 5.5]: Still present as of 4b62d5c for the ax-service call site. I reverted
ax-service.ts:345to`/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.tscalls 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 claimedext:device always attaches to the provider'slentSocketPath.