fix: respect scoped simulator sets in ios runner#354
Conversation
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e81569340
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| detached: true, | ||
| }, | ||
| ); | ||
| const simulatorSetRedirect = await acquireXcodebuildSimulatorSetRedirect(device); |
There was a problem hiding this comment.
ensureRunnerSession now acquires a long-lived acquireXcodebuildSimulatorSetRedirect handle, but abortAllIosRunnerSessions only kills child processes and never invokes releaseXcodebuildSimulatorSetRedirect. In the client-disconnect path (daemon/transport.ts), this leaves xctest-device-set.lock owned by the still-alive daemon PID, so later scoped simulator commands can block until the 30s lock timeout and fail even though no runner session is active anymore. Please ensure the abort flow also releases redirect handles (or routes through the same stop path that does).
Useful? React with 👍 / 👎.
| const simulatorSetPath = resolveIosSimulatorDeviceSetPath(device.simulatorSetPath); | ||
| if (!simulatorSetPath) { | ||
| return null; |
There was a problem hiding this comment.
Returning early when simulatorSetPath is absent skips lock acquisition entirely, so unscoped simulator xcodebuild runs do not coordinate with a concurrent scoped run that has redirected ~/Library/Developer/XCTestDevices to another set. In that overlap, the unscoped run can resolve destinations against the redirected set and fail to find its UDID. Unscoped simulator runner invocations should also participate in the lock (or otherwise guarantee the default XCTest device-set mapping) to avoid cross-session interference.
Useful? React with 👍 / 👎.
|
Addressed the main hardening points from review in follow-up commit
Validation rerun after the changes:
I did not add a retry loop around non-agent external interference during the swap. The temp-symlink + rename path narrows that window, but a fully robust answer there would need a broader strategy than the in-repo lock alone. |
…ct internals Address P1 review feedback: abortAllIosRunnerSessions now releases simulator-set redirect handles after killing processes, preventing stale locks from blocking subsequent scoped simulator commands. Simplify redirect internals: - Store redirect handle directly on session instead of wrapping in closure - Simplify sameResolvedPath to direct comparison instead of set intersection - Collapse double liveness check in clearStaleXcodebuildSimulatorSetLock https://claude.ai/code/session_01VsAUdPKbQoGbwvftBZtXfB
2 participants
Summary
Closes #353.
Fix the iOS XCUITest runner so
--ios-simulator-device-setalso works for runner-backed commands such aswait,find,press, andsnapshot.The regression was that simulator-scoped
simctlcalls already respected the custom device set, but the runner’sxcodebuild build-for-testingandtest-without-buildingpaths still resolved destinations through Xcode’s XCTest device-set location. That meant a simulator created in a custom set could be opened successfully, but runner commands failed becausexcodebuildcould not find the UDID.This change redirects the XCTest device-set path to the requested simulator set for the lifetime of the runner build/session, restores the original state afterward, and hardens the behavior with:
agent-deviceruns do not trample the same XCTest device-set pathTouched files: 3
Scope stayed within the iOS runner command family.
Validation
pnpm formatpnpm check:unit