diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d22b0d3..a5058e75 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ - Added `toggle_connect_hardware_keyboard` tool to toggle the iOS Simulator hardware keyboard connection ([#346](https://github.com/getsentry/XcodeBuildMCP/issues/346)). - Fixed `xcode_tools_bridge_disconnect` immediately re-syncing proxied tools after a manual disconnect ([#343](https://github.com/getsentry/XcodeBuildMCP/issues/343)). - Stopped suggesting an unsupported `--device-id`/`deviceId` argument in the `device list` next-step hint for `device build`/`build_device`; device targeting flows through session defaults ([#350](https://github.com/getsentry/XcodeBuildMCP/pull/350) by [@MukundaKatta](https://github.com/MukundaKatta)). +- Replaced blocking `execSync`/`readFileSync`/`unlinkSync` in the device name resolver with async process/file operations and background cache refresh, avoiding event-loop blocking during device name formatting ([#333](https://github.com/getsentry/XcodeBuildMCP/issues/333)). ## [2.3.2] diff --git a/src/utils/__tests__/device-name-resolver.test.ts b/src/utils/__tests__/device-name-resolver.test.ts new file mode 100644 index 00000000..1d5785d1 --- /dev/null +++ b/src/utils/__tests__/device-name-resolver.test.ts @@ -0,0 +1,108 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const { execFileMock, readFileMock, unlinkMock } = vi.hoisted(() => ({ + execFileMock: vi.fn(), + readFileMock: vi.fn(), + unlinkMock: vi.fn(), +})); + +vi.mock('node:child_process', () => ({ + execFile: execFileMock, +})); + +vi.mock('node:fs/promises', () => ({ + readFile: readFileMock, + unlink: unlinkMock, +})); + +async function flushAsyncWork(): Promise { + await Promise.resolve(); + await new Promise((resolve) => setTimeout(resolve, 0)); +} + +describe('device-name-resolver', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.resetModules(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it('loads device names asynchronously and caches resolved names', async () => { + execFileMock.mockImplementation( + ( + _file: string, + _args: string[], + _options: unknown, + callback: (error: Error | null, stdout: string, stderr: string) => void, + ) => { + callback(null, '', ''); + return {}; + }, + ); + + readFileMock.mockResolvedValue( + JSON.stringify({ + result: { + devices: [ + { + identifier: 'device-1', + deviceProperties: { name: 'iPhone 15 Pro' }, + hardwareProperties: { udid: 'udid-1' }, + }, + ], + }, + }), + ); + unlinkMock.mockResolvedValue(undefined); + + const { resolveDeviceName, formatDeviceId } = await import('../device-name-resolver.ts'); + + expect(resolveDeviceName('device-1')).toBeUndefined(); + expect(execFileMock).toHaveBeenCalledTimes(1); + + await flushAsyncWork(); + + expect(resolveDeviceName('device-1')).toBe('iPhone 15 Pro'); + expect(resolveDeviceName('udid-1')).toBe('iPhone 15 Pro'); + expect(formatDeviceId('device-1')).toBe('iPhone 15 Pro (device-1)'); + expect(readFileMock).toHaveBeenCalledTimes(1); + expect(unlinkMock).toHaveBeenCalledTimes(1); + }); + + it('does not spawn duplicate refreshes while one is in flight', async () => { + let callback: ((error: Error | null, stdout: string, stderr: string) => void) | undefined; + + execFileMock.mockImplementation( + ( + _file: string, + _args: string[], + _options: unknown, + cb: (error: Error | null, stdout: string, stderr: string) => void, + ) => { + callback = cb; + return {}; + }, + ); + + readFileMock.mockResolvedValue( + JSON.stringify({ + result: { devices: [{ identifier: 'device-2', deviceProperties: { name: 'iPad' } }] }, + }), + ); + unlinkMock.mockResolvedValue(undefined); + + const { resolveDeviceName } = await import('../device-name-resolver.ts'); + + expect(resolveDeviceName('device-2')).toBeUndefined(); + expect(resolveDeviceName('device-2')).toBeUndefined(); + expect(execFileMock).toHaveBeenCalledTimes(1); + + callback?.(null, '', ''); + await flushAsyncWork(); + + expect(resolveDeviceName('device-2')).toBe('iPad'); + }); +}); diff --git a/src/utils/device-name-resolver.ts b/src/utils/device-name-resolver.ts index 876607a6..728b7fd1 100644 --- a/src/utils/device-name-resolver.ts +++ b/src/utils/device-name-resolver.ts @@ -1,12 +1,15 @@ -import { execSync } from 'node:child_process'; -import { readFileSync, unlinkSync } from 'node:fs'; +import { execFile } from 'node:child_process'; +import { readFile, unlink } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; +import { promisify } from 'node:util'; const CACHE_TTL_MS = 30_000; +const execFileAsync = promisify(execFile); let cachedDevices: Map | null = null; let cacheTimestamp = 0; +let loadPromise: Promise | null = null; interface DeviceCtlEntry { identifier: string; @@ -14,50 +17,77 @@ interface DeviceCtlEntry { hardwareProperties?: { udid?: string }; } -function loadDeviceNames(): Map { - if (cachedDevices && Date.now() - cacheTimestamp < CACHE_TTL_MS) { - return cachedDevices; - } +function cacheIsFresh(): boolean { + return cachedDevices !== null && Date.now() - cacheTimestamp < CACHE_TTL_MS; +} +function createDeviceMap(data: { result?: { devices?: DeviceCtlEntry[] } }): Map { const map = new Map(); - const tmpFile = join(tmpdir(), `devicectl-list-${process.pid}.json`); - - try { - execSync(`xcrun devicectl list devices --json-output ${tmpFile}`, { - encoding: 'utf8', - timeout: 10_000, - stdio: 'pipe', - }); - - const data = JSON.parse(readFileSync(tmpFile, 'utf8')) as { - result?: { devices?: DeviceCtlEntry[] }; - }; - - for (const device of data.result?.devices ?? []) { - const name = device.deviceProperties.name; - map.set(device.identifier, name); - if (device.hardwareProperties?.udid) { - map.set(device.hardwareProperties.udid, name); - } + + for (const device of data.result?.devices ?? []) { + const name = device.deviceProperties.name; + map.set(device.identifier, name); + if (device.hardwareProperties?.udid) { + map.set(device.hardwareProperties.udid, name); } - } catch { - // Device list unavailable -- return empty map, will fall back to UUID only - } finally { + } + + return map; +} + +async function refreshDeviceNames(): Promise { + if (cacheIsFresh()) { + return; + } + + if (loadPromise) { + return loadPromise; + } + + const tmpFile = join(tmpdir(), `devicectl-list-${process.pid}-${Date.now()}.json`); + + loadPromise = (async () => { try { - unlinkSync(tmpFile); + await execFileAsync('xcrun', ['devicectl', 'list', 'devices', '--json-output', tmpFile], { + encoding: 'utf8', + timeout: 10_000, + }); + + const data = JSON.parse(await readFile(tmpFile, 'utf8')) as { + result?: { devices?: DeviceCtlEntry[] }; + }; + + cachedDevices = createDeviceMap(data); + cacheTimestamp = Date.now(); } catch { - // ignore + // Device list unavailable -- keep existing cache and fall back to UUID only + if (cachedDevices === null) { + cachedDevices = new Map(); + cacheTimestamp = Date.now(); + } + } finally { + loadPromise = null; + try { + await unlink(tmpFile); + } catch { + // ignore + } } - } + })(); - cachedDevices = map; - cacheTimestamp = Date.now(); - return map; + return loadPromise; +} + +function ensureDeviceNamesRefresh(): void { + void refreshDeviceNames(); } export function resolveDeviceName(deviceId: string): string | undefined { - const names = loadDeviceNames(); - return names.get(deviceId); + if (!cacheIsFresh()) { + ensureDeviceNamesRefresh(); + } + + return cachedDevices?.get(deviceId); } export function formatDeviceId(deviceId: string): string {