[Fix] Retry simctl against the testing device set on Invalid device by nGervasyuk · Pull Request #8 · betterme-dev/Mussel

nGervasyuk · 2026-09-07T06:14:04Z

Проблема

xcodebuild при нативній паралелізації створює клони симуляторів в окремому device set (~/Library/Developer/XCTestDevices). Звичайний simctl <cmd> <udid> таких девайсів не бачить і повертає Invalid device — причому в stderr, який сервер не захоплював. У результаті uninstall/privacy reset мовчки не виконувались, тести стартували на брудному стейті попереднього тесту (~75% фейлів у пілоті IOS_BETTER_ME).

Рішення

run(command:) захоплює stderr разом зі stdout — помилки simctl тепер видно у відповіді сервера і в лозі;
при Invalid device команда ретраїться з --set testing;
сервер запам'ятовує, які UDID живуть у testing set — наступні команди для того ж симулятора йдуть одразу правильною командою, без зайвої спроби.

Верифікація

Пілотний ран (Band, 6 клонів у testing set): 758 успішних ретраїв, фейл-рейт впав з ~75% до ~5%. Кеш перевірено локально: перший виклик — ретрай, другий — одразу testing set.

Контекст

CI зараз працює через multi_scan (клони в дефолтному set), тож у штатному режимі фолбек не активується і нічого не коштує. Це страховка від класу «мовчки нічого не зробив» — щоб будь-який майбутній запуск із нативною паралеллю (Xcode, локально, експерименти) працював коректно. Зібраний із цієї гілки бінарник комітиться в IOS_BETTER_ME (scripts/MusselServer) у парному PR — мержити цей PR перед ним.

xcodebuild parallel testing creates simulator clones in a separate device set (~/Library/Developer/XCTestDevices) that plain simctl cannot see, so uninstall/privacy commands silently failed for those clones. Capture stderr (where simctl reports Invalid device) and retry the command with --set testing before giving up.

The Invalid-device retry doubled every simctl call for parallel-testing clones. Remember simulators that resolved via --set testing and build their commands against that set right away, so only the first command per clone pays the extra attempt.

Copilot

🟡 Changes recommended

The push endpoint currently executes the simctl push command twice (including once after deleting the payload file), which can cause incorrect behavior and misleading responses.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens MusselServer’s simctl command execution so simulator management works reliably when Xcode’s native parallel testing uses the separate XCTestDevices device set, and makes simctl failures visible by capturing stderr.

Changes:

Capture stderr along with stdout for executed commands to surface simctl errors in server responses/logs.
Retry xcrun simctl ... commands with --set testing when output contains Invalid device, and cache simulator UDIDs that require the testing device set.

File summaries

File	Description
Sources/MusselServer/ServerManager.swift	Adds `stderr` capture, retry-with-testing-set logic, and a per-simulator cache to avoid repeated failing `simctl` calls.

Review details

Files reviewed: 1/1 changed files
Comments generated: 2
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Commands are built as argv arrays and executed via /usr/bin/env instead of sh -c, so values from request JSON can never inject shell commands; the testing-set retry inserts --set testing into the argv. The push endpoint also ran simctl push twice — the second time after deleting the payload file — now it runs once and reports that result.

Copilot

🟡 Changes recommended

The new process-launch implementation relies on /usr/bin/env/PATH resolution and deprecated Process APIs, which can cause command execution to fail in constrained runtime environments (e.g., CI/daemon launches).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 1/1 changed files
Comments generated: 1
Review effort level: Lite

/usr/bin/env resolved xcrun through the server's PATH, which can break under a minimal launch environment; simctl commands now execute /usr/bin/xcrun directly, with env kept as a fallback for anything else.

Copilot

🔵 Needs a closer look

launch(command:) still doesn’t surface non-zero exit status when a command produces no output, which can reintroduce silent failures (the core reliability issue this PR aims to eliminate).

Review details

Suppressed comments (1)

Sources/MusselServer/ServerManager.swift:243

launch(command:) only returns captured output; if the process exits non‑zero but produces no stdout/stderr, the response/logs will still look like a successful (empty) result. Including the exit status (and waiting for termination) would prevent “silent no-op” failures from reappearing.

        task.launch()
        if let result = NSString(data: file.readDataToEndOfFile(), encoding: String.Encoding.utf8.rawValue) {
            print(result as String)
            return result as String
        } else {

Files reviewed: 1/1 changed files
Comments generated: 0 new
Review effort level: Lite

…tput

Copilot

🔵 Needs a closer look

launch(command:) still uses deprecated Process.launch()/launchPath and lacks robust startup error handling, which can undermine the PR’s reliability goals in minimal/CI environments.

Review details

Suppressed comments (1)

Sources/MusselServer/ServerManager.swift:242

Process.launch()/launchPath are deprecated and don’t surface failures to start the process. Switching to executableURL + try task.run() lets you handle startup errors (e.g., missing executable/permissions) and avoids deprecation warnings while keeping the same stdout/stderr capture behavior.

        let file = pipe.fileHandleForReading
        task.launch()
        let output = NSString(data: file.readDataToEndOfFile(), encoding: String.Encoding.utf8.rawValue) as String?
        task.waitUntilExit()

Files reviewed: 1/1 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot

🔵 Needs a closer look

The new testingSetVariant(of:) can duplicate an existing --set argument, potentially producing an invalid simctl argv in cached paths.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

Sources/MusselServer/ServerManager.swift:209

testingSetVariant(of:) always inserts --set testing even if the caller already provided a --set argument. If run(command:simulatorId:) is called with a pre-set simctl command (or if future code adds one), this will produce an invalid argv like simctl --set testing --set <...> and can break the command.

Files reviewed: 1/1 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot

🔵 Needs a closer look

The new retry/cache and command execution logic has a couple of operational edge cases (exit status visibility and unbounded cache growth) that should be addressed to fully meet the “no silent failures” goal.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

Sources/MusselServer/ServerManager.swift:224

testingSetSimulatorIds is an unbounded in-memory cache and is never pruned; in long-lived server processes that see many ephemeral simulator clones, this can grow without limit. Consider applying a simple upper bound / reset to avoid unbounded memory growth.
Sources/MusselServer/ServerManager.swift:259
launch(command:) can still return a non-zero exit status without any explicit indication when the command produced some output; callers may treat a failing simctl invocation as success unless they parse the text. Consider surfacing the exit status for any non-zero terminationStatus (and keeping the existing special-case for empty output).

Files reviewed: 1/1 changed files
Comments generated: 0 new
Review effort level: Lite

…s always

Copilot

🔵 Needs a closer look

The new --set detection uses Array.contains("--set"), which can be accidentally triggered by user-supplied argument values and incorrectly disable the intended retry/variant behavior.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

Sources/MusselServer/ServerManager.swift:197

The retry guard uses effectiveCommand.contains("--set"), which can be accidentally tripped by any argument value equal to --set (e.g., a user-supplied URL), disabling the retry even though the simctl device set option is not actually present. Since --set is only a valid simctl global option when it appears immediately after simctl, check the positional form instead.

This issue also appears on line 206 of the same file.

Sources/MusselServer/ServerManager.swift:208

testingSetVariant(of:) uses command.contains("--set") to detect an existing device-set option. This can incorrectly treat an argument value (e.g., a URL or path) equal to --set as if the global simctl option is present, preventing insertion of --set testing. Check the expected positional form (["xcrun","simctl","--set",...]) instead.

        guard command.count >= 2, command[0] == "xcrun", command[1] == "simctl",
              !command.contains("--set")
        else { return command }

Files reviewed: 1/1 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot

🟢 Approval recommended

The changes are localized and address a concrete operational failure mode, with only minor logging-message clarity nits noted.

Review details

Suppressed comments (6)

Previously missed (1) — in code that hasn't changed since the last review.

Sources/MusselServer/ServerManager.swift:54

The response/log message says "Ran command" but run(command:simulatorId:) may transparently add --set testing (or retry with it), so this string can be misleading when debugging failures.

This issue also appears in the following locations of the same file:

line 77
line 98
line 118
line 137
line 157

Sources/MusselServer/ServerManager.swift:77

The response/log message says "Ran command" but run(command:simulatorId:) may transparently add --set testing (or retry with it), so this string can be misleading when debugging failures.

            let responseInfo = "Ran command: \(command.joined(separator: " ")) \n Result:\n \(result ?? "Empty result")"

Sources/MusselServer/ServerManager.swift:118

The response/log message says "Ran command" but run(command:simulatorId:) may transparently add --set testing (or retry with it), so this string can be misleading when debugging failures.

            let responseInfo = "Ran command: \(command.joined(separator: " ")) \n Result:\n \(result ?? "Empty result")"

Sources/MusselServer/ServerManager.swift:137

The response/log message says "Ran command" but run(command:simulatorId:) may transparently add --set testing (or retry with it), so this string can be misleading when debugging failures.

            let responseInfo = "Ran command: \(command.joined(separator: " ")) \n Result:\n \(result ?? "Empty result")"

Sources/MusselServer/ServerManager.swift:157

The response/log message says "Ran command" but run(command:simulatorId:) may transparently add --set testing (or retry with it), so this string can be misleading when debugging failures.

            let responseInfo = "Ran command: \(command.joined(separator: " ")) \n Result:\n \(result ?? "Empty result")"

Sources/MusselServer/ServerManager.swift:98

The response/log message says "Ran command" but run(command:simulatorId:) may transparently add --set testing (or retry with it), so this string can be misleading when debugging failures.

            let responseInfo = "Ran command: \(command.joined(separator: " ")) \n Result:\n \(result ?? "Empty result")"

Files reviewed: 1/1 changed files
Comments generated: 0 new
Review effort level: Lite

nGervasyuk added 2 commits September 3, 2026 13:57

nGervasyuk self-assigned this Sep 7, 2026

nGervasyuk requested a lite review from Copilot September 7, 2026 06:20

Copilot started reviewing on behalf of nGervasyuk September 7, 2026 06:20 View session