fix(utils): Replace sync device name resolver calls#376
fix(utils): Replace sync device name resolver calls#376cameroncooke wants to merge 1 commit intomainfrom
Conversation
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 <codex@openai.com>
| } finally { | ||
| loadPromise = null; | ||
| try { | ||
| await unlink(tmpFile); | ||
| } catch { | ||
| // ignore | ||
| } | ||
| } |
There was a problem hiding this comment.
Race condition clears loadPromise before unlink completes, allowing duplicate xcrun invocations
In refreshDeviceNames, the IIFE sets loadPromise = null in the finally block before awaiting unlink(tmpFile). Because the assignment happens synchronously at the start of the finally, a concurrent caller invoking refreshDeviceNames during the unlink await will see loadPromise as null and a fresh cache, but if the cache check fails for any reason (e.g., the catch branch did not populate it on a TTL boundary), a second xcrun process can be spawned while the first is still cleaning up. More importantly, the deduplication guarantee is weaker than intended — loadPromise should remain set for the entire lifetime of the returned promise so awaiters all observe the same in-flight work.
Verification
Traced the IIFE assigned to loadPromise: the outer loadPromise = (async () => { ... })() resolves only after the finally block finishes, but inside the finally loadPromise = null runs before await unlink(...). Confirmed there is no other synchronization preventing a new caller from entering the if (loadPromise) check during the unlink await window.
Identified by Warden find-bugs, code-review · XGV-259
| vi.mock('node:child_process', () => ({ | ||
| execFile: execFileMock, | ||
| })); | ||
|
|
||
| vi.mock('node:fs/promises', () => ({ | ||
| readFile: readFileMock, | ||
| unlink: unlinkMock, | ||
| })); |
There was a problem hiding this comment.
Test mocks node:child_process and node:fs/promises directly instead of injecting executors
The test uses vi.mock('node:child_process') and vi.mock('node:fs/promises') to intercept execFile, readFile, and unlink rather than injecting a CommandExecutor/FileSystemExecutor as the project's test guardrails require. A repo-wide grep shows this is the only test file mocking node:child_process this way — every other unit test goes through createMockExecutor from src/test-utils/mock-executors.ts. The underlying cause is that src/utils/device-name-resolver.ts imports execFile/readFile/unlink directly at module scope and offers no injection seam, so the test cannot follow the standard pattern; this couples the suite to Node internals and bypasses the executor mock helpers the guardrails mandate.
Verification
Read src/utils/device-name-resolver.ts and confirmed it directly imports execFile from node:child_process and readFile/unlink from node:fs/promises with no executor parameter. Read src/test-utils/mock-executors.ts and confirmed createMockExecutor / FileSystemExecutor are the project's standard injection helpers. Ran a grep for vi.mock('node:child_process') across src/ — this new test is the only match, confirming it deviates from the established pattern.
Identified by Warden xcodebuildmcp-test-boundary-review · TZF-HCM
commit: |
|
Coordinator follow-up: this draft introduces a behavior regression on cold cache (first device ID format can fall back to raw UUID). Preserving current behavior while removing sync calls requires making the rendering/header formatting path async (renderers, pipeline header param generation, and transcript interfaces), which is broader than a low-risk change for #333. Closing this draft as not low-hanging. |
|
Closing per coordinator review: not low-hanging without a broader async rendering cascade. |
Replace synchronous device-name resolution calls with async alternatives
Issue #333 flagged that
device-name-resolverblocks the Node event loop by usingexecSyncand sync fs calls to runxcrun devicectl list devices. This change switches to async child-process and fs operations while keeping the external API stable for existing call sites.I considered converting all resolver consumers to fully async return paths, but that would cascade through rendering and pipeline code and increase risk. Instead, this keeps the current sync call shape and performs the refresh in the background with in-flight deduplication, then serves cached names.
This PR also adds focused tests for async loading/caching behavior and updates the changelog entry under Unreleased.
Fixes #333