From 9a57de84cc743a8aa7645d89c502a6590fe22ea4 Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Tue, 28 Apr 2026 22:46:45 +0100 Subject: [PATCH] fix(utils): Replace sync device name resolver calls Convert device-name-resolver to use async child process and fs operations so resolving display names no longer blocks the Node event loop. Keep the public resolver API synchronous by triggering async refresh in background and serving cached values when available. Add focused tests for async loading, cache population, and in-flight deduplication. Fixes #333 Co-Authored-By: OpenAI Codex --- CHANGELOG.md | 1 + .../__tests__/device-name-resolver.test.ts | 108 ++++++++++++++++++ src/utils/device-name-resolver.ts | 102 +++++++++++------ 3 files changed, 175 insertions(+), 36 deletions(-) create mode 100644 src/utils/__tests__/device-name-resolver.test.ts 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 {