From 316e240b5b9588b6b2b9916e40934ce20dde269c Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Tue, 1 Sep 2026 15:38:55 +0200 Subject: [PATCH 1/6] feat(config): add downloads policy gating component installation (#67) Introduces `config.downloads.policy` (`"never" | "on-request" | "always"`, default `"on-request"`) alongside `acceptAndroidLicenses` and `timeoutMs`, plus the pure `effectiveAllowDownload(policy, requested)` reducer that combines the policy with a lease request's own `--allow-download` / `allow_download` flag. The daemon resolves this once in `#requestLease` before calling `leases.request`: `"never"` overrides an explicit `true` back to forbidden (and the resulting RuntimeMissingError names `downloads.policy` in its message), `"always"` grants permission without the caller having to ask, and `"on-request"` defers to the request's flag exactly as before. Warm-pool provisioning and startup convergence never resolve a new spec, so they remain unaffected by policy under any setting. Updates docs/agent-rules/safety.md (rule 4) and docs/ARCHITECTURE.md ("Device requests") to describe the policy resolution point and the warm-pool/startup no-download guarantee, and adds the required `downloads` field to every hand-built `Config` test fixture. --- docs/ARCHITECTURE.md | 16 +++- docs/agent-rules/safety.md | 8 +- src/cli/index.test.ts | 1 + src/core/acquisition-planner.test.ts | 1 + src/core/cleanup/idle-destroy.test.ts | 1 + src/core/cleanup/idle-shutdown.test.ts | 1 + src/core/config.test.ts | 95 ++++++++++++++++++- src/core/config.ts | 39 ++++++++ src/core/doctor.test.ts | 1 + src/core/index.ts | 9 +- .../lease-acquisition-coordinator.test.ts | 1 + src/core/lease-engine.test.ts | 1 + src/core/lease-health-monitor.test.ts | 1 + src/core/nuke.test.ts | 1 + src/core/reaper.test.ts | 1 + src/core/warm-pool-coordinator.test.ts | 1 + src/daemon/server.test.ts | 81 +++++++++++++++- src/daemon/server.ts | 18 +++- 18 files changed, 269 insertions(+), 8 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 14831ce..6801725 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -566,5 +566,17 @@ logger straight from the default log path at a fixed level, falling back to Required to identify a device: **platform + device model + OS version**. OS defaults to the newest runtime already installed on the machine. If the requested runtime / system image is not installed, the lease fails with a -clear error unless `--allow-download` is passed (downloads are multi-GB and -must never be triggered implicitly). +clear error unless downloads are permitted for that request (downloads are +multi-GB and must never be triggered implicitly). + +Permission comes from `config.downloads.policy`, resolved once, in the +daemon, before a request ever reaches the acquisition path: `"never"` +forbids installs outright, even over an explicit `--allow-download` / +`allow_download`; `"always"` grants it to every explicit lease request +without the caller having to ask; `"on-request"` (the default) defers to +the request's own flag, which is today's behavior byte-for-byte. Only an +explicit lease request (`LeaseEngine#request`) can carry download +permission to a driver's `resolveSpec` — warm-pool provisioning and startup +convergence reuse specs already committed to the registry and never call +`resolveSpec` themselves, so neither can trigger a download regardless of +policy. diff --git a/docs/agent-rules/safety.md b/docs/agent-rules/safety.md index 61aea38..5efa169 100644 --- a/docs/agent-rules/safety.md +++ b/docs/agent-rules/safety.md @@ -27,7 +27,13 @@ never enforce them only inside an individual rule or driver. over a read-only registry view returning proposed actions. A rule that executes side effects directly is a bug regardless of what it does. 4. **No implicit multi-GB downloads.** Missing runtimes / system images fail - the request unless `--allow-download` was explicitly passed. + the request unless `--allow-download` (or MCP's `allow_download`) was + explicitly passed, or `downloads.policy: "always"` is set in config -- + both count as the required explicit consent, and `downloads.policy: + "never"` overrides either one back to forbidden. Warm-pool provisioning + and startup convergence never trigger a download under any policy: they + only ever reuse specs already committed to the registry, never resolve a + new one. 5. **Destructive CLI commands confirm or require `--yes`** (`release --all`, `nuke`). `cleanup` must always support `--dry-run`. 6. **Every destructive action is attributable.** Log/emit which rule or diff --git a/src/cli/index.test.ts b/src/cli/index.test.ts index 07ad286..3b3758d 100644 --- a/src/cli/index.test.ts +++ b/src/cli/index.test.ts @@ -1046,6 +1046,7 @@ function testConfig(): Config { stableObservations: 2, }, stalledTransition: { thresholdMultiplier: 3, minimumThresholdMs: 60_000 }, + downloads: { policy: "on-request", acceptAndroidLicenses: false, timeoutMs: 1_200_000 }, idle: { deleteAfterMs: 60_000, shutdownAfterMs: 10_000 }, lease: { detachedTtlMs: 60_000, heldTtlBackstopMs: 60_000, heartbeatIntervalMs: 15_000 }, capacity: { diff --git a/src/core/acquisition-planner.test.ts b/src/core/acquisition-planner.test.ts index 9eecea8..d13d9d6 100644 --- a/src/core/acquisition-planner.test.ts +++ b/src/core/acquisition-planner.test.ts @@ -21,6 +21,7 @@ const config: Config = { stableObservations: 2, }, stalledTransition: { thresholdMultiplier: 3, minimumThresholdMs: 60_000 }, + downloads: { policy: "on-request", acceptAndroidLicenses: false, timeoutMs: 1_200_000 }, idle: { deleteAfterMs: 60_000, shutdownAfterMs: 10_000 }, warmPool: { quarantine: { diff --git a/src/core/cleanup/idle-destroy.test.ts b/src/core/cleanup/idle-destroy.test.ts index 67dd3c5..253cded 100644 --- a/src/core/cleanup/idle-destroy.test.ts +++ b/src/core/cleanup/idle-destroy.test.ts @@ -17,6 +17,7 @@ const config: Config = { stableObservations: 2, }, stalledTransition: { thresholdMultiplier: 3, minimumThresholdMs: 60_000 }, + downloads: { policy: "on-request", acceptAndroidLicenses: false, timeoutMs: 1_200_000 }, idle: { deleteAfterMs: 30_000, shutdownAfterMs: 10_000 }, warmPool: { quarantine: { diff --git a/src/core/cleanup/idle-shutdown.test.ts b/src/core/cleanup/idle-shutdown.test.ts index ef3c800..5d305d1 100644 --- a/src/core/cleanup/idle-shutdown.test.ts +++ b/src/core/cleanup/idle-shutdown.test.ts @@ -15,6 +15,7 @@ const config: Config = { stableObservations: 2, }, stalledTransition: { thresholdMultiplier: 3, minimumThresholdMs: 60_000 }, + downloads: { policy: "on-request", acceptAndroidLicenses: false, timeoutMs: 1_200_000 }, idle: { deleteAfterMs: 30_000, shutdownAfterMs: 10_000 }, warmPool: { quarantine: { diff --git a/src/core/config.test.ts b/src/core/config.test.ts index a639514..266be8c 100644 --- a/src/core/config.test.ts +++ b/src/core/config.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it, vi } from "vitest"; import { MemoryFilesystem, FakeSystemStats } from "../ports/index.js"; import type { ResourceStrategyOptions } from "./capacity/index.js"; -import { type Config, loadConfig } from "./index.js"; +import { type Config, effectiveAllowDownload, loadConfig } from "./index.js"; /** Narrows the capacity block for assertions on resource-strategy configs. */ function resourceOptions(config: Config): ResourceStrategyOptions { @@ -72,6 +72,7 @@ describe("loadConfig", () => { }, }, log: { level: "info", rotateBytes: 5 * 1024 * 1024 }, + downloads: { policy: "on-request", acceptAndroidLicenses: false, timeoutMs: 1_200_000 }, warmPool: { quarantine: { maxRetries: 3, @@ -356,6 +357,81 @@ describe("loadConfig", () => { expect(warn).toHaveBeenCalledWith('Unknown config key: "health.maxBoltCount"'); }); + it("applies a file-level downloads override", async () => { + const filesystem = new MemoryFilesystem(); + await filesystem.mkdirp("/home/agent/.simlock"); + await filesystem.writeFileAtomic( + configPath, + JSON.stringify({ + downloads: { policy: "always", acceptAndroidLicenses: true, timeoutMs: 60_000 }, + }), + ); + + const config = await loadConfig({ configPath, filesystem, systemStats: createStats() }); + expect(config.downloads).toEqual({ + policy: "always", + acceptAndroidLicenses: true, + timeoutMs: 60_000, + }); + }); + + it("applies an override-level downloads policy over the file value", async () => { + const filesystem = new MemoryFilesystem(); + await filesystem.mkdirp("/home/agent/.simlock"); + await filesystem.writeFileAtomic( + configPath, + JSON.stringify({ downloads: { policy: "never" } }), + ); + + const config = await loadConfig({ + configPath, + filesystem, + overrides: { downloads: { policy: "always" } }, + systemStats: createStats(), + }); + expect(config.downloads.policy).toBe("always"); + }); + + it("rejects a downloads.policy outside the known set", async () => { + const filesystem = new MemoryFilesystem(); + await filesystem.mkdirp("/home/agent/.simlock"); + await filesystem.writeFileAtomic( + configPath, + JSON.stringify({ downloads: { policy: "sometimes" } }), + ); + + await expect( + loadConfig({ configPath, filesystem, systemStats: createStats() }), + ).rejects.toThrow("downloads.policy"); + }); + + it("rejects a non-boolean downloads.acceptAndroidLicenses", async () => { + const filesystem = new MemoryFilesystem(); + await filesystem.mkdirp("/home/agent/.simlock"); + await filesystem.writeFileAtomic( + configPath, + JSON.stringify({ downloads: { acceptAndroidLicenses: "yes" } }), + ); + + await expect( + loadConfig({ configPath, filesystem, systemStats: createStats() }), + ).rejects.toThrow("downloads.acceptAndroidLicenses"); + }); + + it.each([ + [{ downloads: { timeoutMs: 0 } }, "downloads.timeoutMs"], + [{ downloads: { timeoutMs: -1 } }, "downloads.timeoutMs"], + [{ downloads: { timeoutMs: "1200000" } }, "downloads.timeoutMs"], + ])("rejects a non-positive or malformed downloads.timeoutMs", async (contents, path) => { + const filesystem = new MemoryFilesystem(); + await filesystem.mkdirp("/home/agent/.simlock"); + await filesystem.writeFileAtomic(configPath, JSON.stringify(contents)); + + await expect( + loadConfig({ configPath, filesystem, systemStats: createStats() }), + ).rejects.toThrow(path); + }); + it("applies a file-level stalledTransition override", async () => { const filesystem = new MemoryFilesystem(); await filesystem.mkdirp("/home/agent/.simlock"); @@ -536,3 +612,20 @@ describe("loadConfig legacy capacity keys", () => { expect(warn).toHaveBeenCalledWith(expect.stringContaining("Ignoring limits")); }); }); + +describe("effectiveAllowDownload", () => { + it("grants downloads for every request under the always policy", () => { + expect(effectiveAllowDownload("always", false)).toBe(true); + expect(effectiveAllowDownload("always", true)).toBe(true); + }); + + it("forbids downloads for every request under the never policy, even an explicit true", () => { + expect(effectiveAllowDownload("never", false)).toBe(false); + expect(effectiveAllowDownload("never", true)).toBe(false); + }); + + it("defers to the request's own flag under the on-request policy", () => { + expect(effectiveAllowDownload("on-request", false)).toBe(false); + expect(effectiveAllowDownload("on-request", true)).toBe(true); + }); +}); diff --git a/src/core/config.ts b/src/core/config.ts index b426c14..4dee9af 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -28,8 +28,23 @@ import { const DEFAULT_CONFIG_PATH = "~/.simlock/config.json"; +/** + * `never` forbids installs even when a request passes `--allow-download` (locked-down + * machines/CI). `on-request` (default) preserves today's contract: install only when the + * request itself carries the flag. `always` lets the daemon install missing components for + * any explicit lease request without the per-request flag. + */ +export type DownloadPolicy = "never" | "on-request" | "always"; + export interface Config { readonly capacity: CapacityConfig; + readonly downloads: { + readonly policy: DownloadPolicy; + /** Explicit legal consent for Android SDK licenses, independent of `policy`. */ + readonly acceptAndroidLicenses: boolean; + /** Per-install timeout; downloads run minutes, not seconds. */ + readonly timeoutMs: number; + }; readonly idle: { readonly shutdownAfterMs: number; readonly deleteAfterMs: number; @@ -131,6 +146,19 @@ export async function loadConfig({ return deepFreeze(merged); } +/** + * Reduces `downloads.policy` and a request's own `--allow-download` / `allow_download` flag + * to the single permission a driver's `resolveSpec` actually sees. `never` overrides an + * explicit `true` on the request -- the whole point of the policy is that it cannot be + * opted back into per request -- and `always` grants permission the request never had to ask + * for. Only `on-request` defers to what the caller asked for, which is today's behavior. + */ +export function effectiveAllowDownload(policy: DownloadPolicy, requested: boolean): boolean { + if (policy === "always") return true; + if (policy === "never") return false; + return requested; +} + /** * Cross-field check: the heartbeat cadence must be frequent enough, relative to the * backstop, that a few missed beats (context compaction, a slow tick) still land well @@ -220,6 +248,11 @@ function defaultConfig(systemStats: SystemStats, strategy: CapacityStrategyName) strategy, config: defaultCapacityOptions(strategy, systemStats), } as CapacityConfig, + downloads: { + policy: "on-request", + acceptAndroidLicenses: false, + timeoutMs: 1_200_000, + }, idle: { shutdownAfterMs: 10 * 60_000, deleteAfterMs: 60 * 60_000, @@ -296,6 +329,7 @@ function validateConfigLayer( } const LOG_LEVELS: readonly LogLevel[] = ["debug", "info", "warn", "error"]; +const DOWNLOAD_POLICIES: readonly DownloadPolicy[] = ["never", "on-request", "always"]; /** * The `capacity.config` validator is the selected strategy's own, so a strategy @@ -310,6 +344,11 @@ function configValidators(strategy: CapacityStrategyName): Record minimumThresholdMs: 1_000, ...stalledTransitionOverrides, }, + downloads: { policy: "on-request", acceptAndroidLicenses: false, timeoutMs: 1_200_000 }, }; } diff --git a/src/core/index.ts b/src/core/index.ts index 6233662..7d1d3e9 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -1,4 +1,11 @@ -export { type Config, type ConfigOverrides, loadConfig } from "./config.js"; +export { + type Config, + type ConfigOverrides, + // fallow-ignore-next-line unused-type -- public Config surface (config.downloads.policy); no in-tree consumer names it directly yet + type DownloadPolicy, + effectiveAllowDownload, + loadConfig, +} from "./config.js"; export { type DeviceRecord, type DeviceSpec, diff --git a/src/core/lease-acquisition-coordinator.test.ts b/src/core/lease-acquisition-coordinator.test.ts index 8237eb9..28eac33 100644 --- a/src/core/lease-acquisition-coordinator.test.ts +++ b/src/core/lease-acquisition-coordinator.test.ts @@ -26,6 +26,7 @@ const request = { model: "iPhone 16", osVersion: "26.5", platform: "ios" } as co function config(maxDevices = 1): Config { return { diskPressure: { freeBytesThreshold: 10 * gibibyte }, + downloads: { acceptAndroidLicenses: false, policy: "on-request", timeoutMs: 1_200_000 }, eventBuffer: { capacity: 100 }, health: { enabled: true, diff --git a/src/core/lease-engine.test.ts b/src/core/lease-engine.test.ts index 73c7120..4ba7734 100644 --- a/src/core/lease-engine.test.ts +++ b/src/core/lease-engine.test.ts @@ -22,6 +22,7 @@ const request = { model: "iPhone 16", osVersion: "26.5", platform: "ios" } as co function config(overrides: Partial = {}): Config { return { diskPressure: { freeBytesThreshold: 10 * gibibyte }, + downloads: { acceptAndroidLicenses: false, policy: "on-request", timeoutMs: 1_200_000 }, eventBuffer: { capacity: 100 }, health: { enabled: true, diff --git a/src/core/lease-health-monitor.test.ts b/src/core/lease-health-monitor.test.ts index c4291fb..7b36d00 100644 --- a/src/core/lease-health-monitor.test.ts +++ b/src/core/lease-health-monitor.test.ts @@ -51,6 +51,7 @@ function config(overrides: Partial = {}): Config { }, log: { level: "info", rotateBytes: 5 * 1024 * 1024 }, stalledTransition: { thresholdMultiplier: 3, minimumThresholdMs: 60_000 }, + downloads: { policy: "on-request", acceptAndroidLicenses: false, timeoutMs: 1_200_000 }, }; } diff --git a/src/core/nuke.test.ts b/src/core/nuke.test.ts index 6101e7c..449dfdb 100644 --- a/src/core/nuke.test.ts +++ b/src/core/nuke.test.ts @@ -156,6 +156,7 @@ function config(): Config { stableObservations: 2, }, stalledTransition: { thresholdMultiplier: 3, minimumThresholdMs: 60_000 }, + downloads: { policy: "on-request", acceptAndroidLicenses: false, timeoutMs: 1_200_000 }, idle: { deleteAfterMs: 10, shutdownAfterMs: 5 }, lease: { detachedTtlMs: 60_000, heldTtlBackstopMs: 60_000, heartbeatIntervalMs: 15_000 }, capacity: { diff --git a/src/core/reaper.test.ts b/src/core/reaper.test.ts index 4984d3b..bdd4487 100644 --- a/src/core/reaper.test.ts +++ b/src/core/reaper.test.ts @@ -55,6 +55,7 @@ function config(): Config { stableObservations: 2, }, stalledTransition: { thresholdMultiplier: 3, minimumThresholdMs: 60_000 }, + downloads: { policy: "on-request", acceptAndroidLicenses: false, timeoutMs: 1_200_000 }, idle: { deleteAfterMs: 30_000, shutdownAfterMs: 10_000 }, lease: { detachedTtlMs: 100, heldTtlBackstopMs: 100, heartbeatIntervalMs: 25 }, capacity: { diff --git a/src/core/warm-pool-coordinator.test.ts b/src/core/warm-pool-coordinator.test.ts index fa1a630..522e2db 100644 --- a/src/core/warm-pool-coordinator.test.ts +++ b/src/core/warm-pool-coordinator.test.ts @@ -26,6 +26,7 @@ const config: Config = { stableObservations: 2, }, stalledTransition: { thresholdMultiplier: 3, minimumThresholdMs: 60_000 }, + downloads: { policy: "on-request", acceptAndroidLicenses: false, timeoutMs: 1_200_000 }, idle: { deleteAfterMs: 60_000, shutdownAfterMs: 10_000 }, warmPool: { quarantine: { diff --git a/src/daemon/server.test.ts b/src/daemon/server.test.ts index dc823b3..7983998 100644 --- a/src/daemon/server.test.ts +++ b/src/daemon/server.test.ts @@ -1073,6 +1073,73 @@ describe("DaemonServer lease heartbeat", () => { }); }); +describe("DaemonServer download policy", () => { + it("grants download permission under the always policy without a per-request flag", async () => { + const clock = new FakeClock(1_000); + const driver = new FakeDriver({ availableOsVersions: [], clock, platform: "ios" }); + const harness = await createHarness({ clock, downloads: { policy: "always" }, driver }); + const client = await createClient(harness.socketPath); + await hello(client); + + const grant = await client.request("lease.request", { + mode: "held", + requesterId: "agent-1", + request: { model: "iPhone 16", osVersion: "26.5", platform: "ios" }, + }); + + expect(grant.ok).toBe(true); + expect(driver.calls.find((call) => call.operation === "resolveSpec")?.arguments[1]).toEqual({ + allowDownload: true, + }); + await client.close(); + }); + + it("withholds download permission under the never policy even when the request asks for it", async () => { + const clock = new FakeClock(1_000); + const driver = new FakeDriver({ availableOsVersions: [], clock, platform: "ios" }); + const harness = await createHarness({ clock, downloads: { policy: "never" }, driver }); + const client = await createClient(harness.socketPath); + await hello(client); + + const response = await client.request("lease.request", { + allowDownload: true, + mode: "held", + requesterId: "agent-1", + request: { model: "iPhone 16", osVersion: "26.5", platform: "ios" }, + }); + + expect(response.ok).toBe(false); + expect(response.error).toMatchObject({ code: "RUNTIME_MISSING" }); + expect(response.error?.message).toContain("downloads.policy"); + expect(driver.calls.find((call) => call.operation === "resolveSpec")?.arguments[1]).toEqual({ + allowDownload: false, + }); + await client.close(); + }); + + it("defers to the request's own flag under the default on-request policy", async () => { + const clock = new FakeClock(1_000); + const driver = new FakeDriver({ availableOsVersions: [], clock, platform: "ios" }); + const harness = await createHarness({ clock, driver }); + const client = await createClient(harness.socketPath); + await hello(client); + + // No allowDownload on the request and the default policy, so this fails exactly as it + // did before the policy existed -- and, unlike the never-policy case above, the message + // is not attributed to configuration, since nothing in config forced the outcome. + const response = await client.request("lease.request", { + mode: "held", + requesterId: "agent-1", + request: { model: "iPhone 16", osVersion: "26.5", platform: "ios" }, + }); + + expect(response.ok).toBe(false); + expect(response.error).toMatchObject({ code: "RUNTIME_MISSING" }); + expect(response.error?.message).not.toContain("downloads.policy"); + await client.close(); + }); +}); + // fallow-ignore-next-line complexity -- a test harness whose branches are all trivial optional-parameter defaulting. async function createHarness( options: { @@ -1084,6 +1151,7 @@ async function createHarness( readonly clock?: FakeClock; readonly converge?: () => Promise; readonly dispose?: () => void; + readonly downloads?: Partial; readonly driver?: FakeDriver; readonly logger?: Logger; readonly settle?: () => Promise; @@ -1115,7 +1183,7 @@ async function createHarness( ...(options.latencyMs === undefined ? {} : { latencyMs: options.latencyMs }), platform: "ios", }); - const config = testConfig(options.lease); + const config = testConfig(options.lease, options.downloads); const engine = new LeaseEngine({ clock, config, @@ -1260,7 +1328,10 @@ function sequence() { return { generate: () => `${next++}` }; } -function testConfig(leaseOverrides?: Partial): Config { +function testConfig( + leaseOverrides?: Partial, + downloadsOverrides?: Partial, +): Config { return { diskPressure: { freeBytesThreshold: 10 * gibibyte }, eventBuffer: { capacity: 100 }, @@ -1273,6 +1344,12 @@ function testConfig(leaseOverrides?: Partial): Config { stableObservations: 2, }, stalledTransition: { thresholdMultiplier: 3, minimumThresholdMs: 60_000 }, + downloads: { + policy: "on-request", + acceptAndroidLicenses: false, + timeoutMs: 1_200_000, + ...downloadsOverrides, + }, idle: { deleteAfterMs: 60_000, shutdownAfterMs: 10_000 }, lease: { detachedTtlMs: 60_000, diff --git a/src/daemon/server.ts b/src/daemon/server.ts index 6ddf38e..c5078cc 100644 --- a/src/daemon/server.ts +++ b/src/daemon/server.ts @@ -5,6 +5,7 @@ import { type DeviceRequest, type LeaseProgress, type LeaseRecord, + effectiveAllowDownload, NoCapacityError, NoDriverError, QueueTimeoutError, @@ -545,6 +546,12 @@ export class DaemonServer { }; const mode = payload.mode === "detached" ? "detached" : "held"; const requesterId = optionalString(payload, "requesterId") ?? this.options.defaultRequesterId; + // The request's own flag is only the input to the policy, not the final answer: `never` + // overrides an explicit `true` (and is what should show up in the eventual "runtime + // missing" message below), `always` grants it without the caller asking. + const requestedAllowDownload = optionalBoolean(payload, "allowDownload") ?? false; + const downloadsPolicy = this.options.config.downloads.policy; + const blockedByDownloadPolicy = downloadsPolicy === "never" && requestedAllowDownload; let progressSocket: IpcConnection | undefined = connection.socket; const disposeProgress = () => { progressSocket = undefined; @@ -554,7 +561,7 @@ export class DaemonServer { let grant; try { grant = await this.options.leases.request(request, { - allowDownload: optionalBoolean(payload, "allowDownload") ?? false, + allowDownload: effectiveAllowDownload(downloadsPolicy, requestedAllowDownload), mode, noWait: optionalBoolean(payload, "noWait") ?? false, onProgress: (progress) => { @@ -565,6 +572,15 @@ export class DaemonServer { requesterId, ...(typeof payload.timeoutMs === "number" ? { timeoutMs: payload.timeoutMs } : {}), }); + } catch (error: unknown) { + // The driver only ever sees the clamped-to-false permission, so its own + // RuntimeMissingError just says "missing" -- it has no way to know a request asked for a + // download and config refused it. Recover that distinction here, the one place that saw + // both sides, rather than teaching the driver about config. + if (blockedByDownloadPolicy && error instanceof RuntimeMissingError) { + error.message = `${error.message} (downloads are disabled by configuration: downloads.policy is "never")`; + } + throw error; } finally { connection.progressDisposers.delete(disposeProgress); connection.progressRequesters.delete(requesterId); From 59ea1807275e353e00513e6a3b259d263fb5f67b Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Tue, 1 Sep 2026 15:55:52 +0200 Subject: [PATCH 2/6] feat(ios): download missing simulator runtimes with pairing-aware resolution (#67) --- docs/ARCHITECTURE.md | 16 +- docs/CLI.md | 9 +- docs/known-pitfalls.md | 27 ++ src/daemon/main.ts | 14 +- .../ios/fixtures/simctl-list-pairing.json | 43 +++ src/drivers/ios/fixtures/simctl-list.json | 38 ++- src/drivers/ios/index.test.ts | 150 +++++++++- src/drivers/ios/index.ts | 278 +++++++++++++++++- 8 files changed, 544 insertions(+), 31 deletions(-) create mode 100644 src/drivers/ios/fixtures/simctl-list-pairing.json diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 6801725..ba6cd21 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -564,10 +564,18 @@ logger straight from the default log path at a fixed level, falling back to ## Device requests Required to identify a device: **platform + device model + OS version**. -OS defaults to the newest runtime already installed on the machine. If the -requested runtime / system image is not installed, the lease fails with a -clear error unless downloads are permitted for that request (downloads are -multi-GB and must never be triggered implicitly). +OS defaults to the newest runtime already installed on the machine that can +actually run the requested model — for iOS specifically, the newest +installed runtime that both falls inside the device type's supported range +(`simctl list devicetypes`' `minRuntimeVersion`/`maxRuntimeVersion`) and +still lists the model in its `supportedDeviceTypes`, not the newest +installed runtime overall (a newer runtime can drop a model, as iOS 26 did +for iPhone XS/XR). If the requested runtime / system image is not +installed, the lease fails with a clear error unless downloads are +permitted for that request (downloads are multi-GB and must never be +triggered implicitly). An OS version outside a model's supported range +fails immediately with the range named in the error — never as an attempted +download, since no download could make it work. Permission comes from `config.downloads.policy`, resolved once, in the daemon, before a request ever reaches the acquisition path: `"never"` diff --git a/docs/CLI.md b/docs/CLI.md index 4be9040..8313fcc 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -89,8 +89,13 @@ simlock lease --platform --device [--os ] - `--no-wait` — fail immediately with exit 11 instead of queueing. - `--allow-download` — permit downloading a missing runtime / system image (multi-GB; never implicit). Without it, a missing runtime is exit 12. - iOS runtimes remain Xcode-managed in v1: `--allow-download` cannot install - them; install the runtime through Xcode first. + For iOS, this runs `xcodebuild -downloadPlatform iOS` under the hood and + only reaches back to iOS 16.0 (a floor of Xcode's own downloader); older + runtimes and unknown device types (which need a newer Xcode) still require + installing/upgrading Xcode by hand. A requested `--os` outside the + device's supported runtime range (e.g. iPhone Xs above iOS 18.x) fails + immediately — no download is ever attempted for a version that could not + work regardless. - `--detach` — detached mode: print the lease result and exit; the lease is TTL-bound and must be renewed with `simlock lease renew`. - `--bind-pid ` — held mode only: watch this pid for death instead of diff --git a/docs/known-pitfalls.md b/docs/known-pitfalls.md index 680dd57..1e0c6fe 100644 --- a/docs/known-pitfalls.md +++ b/docs/known-pitfalls.md @@ -99,3 +99,30 @@ destroys it (registry-only, as always). The device stays visible as `device.purge-failed` still fires as before; `device.quarantined`, `device.quarantine-recovered`, and `device.quarantine-abandoned` are the new follow-up facts (see `docs/EVENTS.md`). + +## iOS runtime downloads: per-request blocking and the bounded-default edge case + +The iOS driver's `resolveSpec` (`src/drivers/ios/index.ts`) can now run +`xcodebuild -downloadPlatform iOS` when a requested runtime is missing and +downloads are permitted. Two things worth knowing about that path: + +**Only the requesting lease waits.** `resolveSpec` runs inside +`LeaseAcquisitionCoordinator#resolveAndDrive`, per request, outside the +serialized decision gate and outside the FIFO head — a slow download (tens +of minutes for a ~7 GB runtime) blocks only the request that triggered it. +Concurrent requests for the *same* missing runtime are deduped behind an +in-driver promise (one `xcodebuild` invocation, all callers await it); a +request for a different model or version proceeds independently and is +never queued behind someone else's download. + +**The bounded-default edge case.** When no `--os` is given and no installed +runtime pairs with the model, the driver has to guess a version to +download: unbounded models (no `maxRuntimeVersion` cap) get a plain +`-downloadPlatform iOS` (latest), but a model with a bounded max (like an +older device type whose newest compatible runtime is a specific release) +gets `-buildVersion ` — just the major +version number, since the exact patch release isn't known offline (Apple's +downloadables index isn't parsed in v1; see `docs/IDEAS.md`). If Xcode +doesn't have a build matching that bare major version, the download fails +and the caller is told to pass `--os ` explicitly rather than +retrying blind. diff --git a/src/daemon/main.ts b/src/daemon/main.ts index 459f372..1514a22 100644 --- a/src/daemon/main.ts +++ b/src/daemon/main.ts @@ -88,7 +88,14 @@ export async function startDaemon(options: StartDaemonOptions = {}): Promise { ).resolves.toEqual(spec); }); - it("rejects an unknown model", async () => { + it("rejects an unknown model, pointing at a newer Xcode", async () => { const driver = createDriver(scriptedListRunner()); + const result = await driver + .resolveSpec({ model: "iPhone 99", platform: "ios" }, { allowDownload: false }) + .catch((error: unknown) => error); - await expect( - driver.resolveSpec({ model: "iPhone 99", platform: "ios" }, { allowDownload: false }), - ).rejects.toBeInstanceOf(UnknownModelError); + expect(result).toBeInstanceOf(UnknownModelError); + expect(result).toMatchObject({ message: expect.stringContaining("newer Xcode") }); }); it("rejects malformed simctl catalog JSON without trusting partial data", async () => { @@ -110,17 +136,125 @@ describe("IosSimctlDriver", () => { ).rejects.toBeInstanceOf(DriverCrashError); }); - it("rejects missing runtimes even when downloads are allowed", async () => { - const driver = createDriver(scriptedListRunner()); + it("rejects a missing runtime without downloading when downloads are not allowed, naming the fix", async () => { + const runner = scriptedListRunner(); + const driver = createDriver(runner); const result = await driver .resolveSpec( - { model: "iPhone 16", osVersion: "27", platform: "ios" }, + { model: "iPhone 16", osVersion: "18.6", platform: "ios" }, + { allowDownload: false }, + ) + .catch((error: unknown) => error); + + expect(result).toBeInstanceOf(RuntimeMissingError); + expect(result).toMatchObject({ + message: expect.stringMatching(/18\.6/), + }); + expect(result).toMatchObject({ + message: expect.stringMatching(/allow-download|downloads\.policy/), + }); + // Never attempted a download: only the initial catalog list call happened. + expect(runner.calls).toHaveLength(1); + }); + + it("rejects an out-of-range OS version before ever considering a download", async () => { + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: pairingFixture } }, + ]); + const driver = createDriver(runner); + + const result = await driver + .resolveSpec( + { model: "iPhone Xs", osVersion: "26.5", platform: "ios" }, { allowDownload: true }, ) .catch((error: unknown) => error); expect(result).toBeInstanceOf(RuntimeMissingError); - expect(result).toMatchObject({ message: expect.stringContaining("install it via Xcode") }); + expect(result).toMatchObject({ + message: expect.stringContaining("iPhone Xs supports iOS 12.0-18.6"), + }); + // No xcodebuild (or any further simctl) call: out-of-range is checked before download logic. + expect(runner.calls).toHaveLength(1); + }); + + it("selects the newest installed runtime that actually pairs with the model, not the newest overall", async () => { + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: pairingFixture } }, + ]); + const driver = createDriver(runner); + + // iOS 26.5 is newer and installed, but only iOS 18.4 still lists iPhone Xs as supported. + await expect( + driver.resolveSpec({ model: "iPhone Xs", platform: "ios" }, { allowDownload: false }), + ).resolves.toEqual({ model: "iPhone Xs", osVersion: "18.4", platform: "ios" }); + }); + + it("refuses to auto-download a runtime older than the iOS 16.0 floor", async () => { + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: pairingFixture } }, + ]); + const driver = createDriver(runner); + + const result = await driver + .resolveSpec( + { model: "iPhone 7", osVersion: "13.0", platform: "ios" }, + { allowDownload: true }, + ) + .catch((error: unknown) => error); + + expect(result).toBeInstanceOf(DriverCrashError); + expect(result).toMatchObject({ message: expect.stringContaining("16.0") }); + // Range check passed (13.0 is within iPhone 7's 9.0-15.0), but no xcodebuild call was made. + expect(runner.calls).toHaveLength(1); + }); + + it("downloads a missing in-range runtime via xcodebuild and re-scans the catalog", async () => { + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixture } }, + { + match: { + command: "xcodebuild", + args: ["-downloadPlatform", "iOS", "-buildVersion", "18.6"], + }, + }, + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixtureAfterDownload } }, + ]); + const driver = createDriver(runner); + + await expect( + driver.resolveSpec( + { model: "iPhone 16", osVersion: "18.6", platform: "ios" }, + { allowDownload: true }, + ), + ).resolves.toEqual({ model: "iPhone 16", osVersion: "18.6", platform: "ios" }); + expect(runner.calls.map((call) => call.command)).toEqual(["xcrun", "xcodebuild", "xcrun"]); + }); + + it("dedupes concurrent resolveSpec calls for the same missing runtime behind one xcodebuild invocation", async () => { + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixture } }, + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixture } }, + { + match: { + command: "xcodebuild", + args: ["-downloadPlatform", "iOS", "-buildVersion", "18.6"], + }, + }, + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixtureAfterDownload } }, + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixtureAfterDownload } }, + ]); + const driver = createDriver(runner); + const request = { model: "iPhone 16", osVersion: "18.6", platform: "ios" } as const; + + const [first, second] = await Promise.all([ + driver.resolveSpec(request, { allowDownload: true }), + driver.resolveSpec(request, { allowDownload: true }), + ]); + + expect(first).toEqual({ model: "iPhone 16", osVersion: "18.6", platform: "ios" }); + expect(second).toEqual({ model: "iPhone 16", osVersion: "18.6", platform: "ios" }); + expect(runner.calls.filter((call) => call.command === "xcodebuild")).toHaveLength(1); }); it("provisions with the exact simctl argv and returns opaque iOS driver data", async () => { diff --git a/src/drivers/ios/index.ts b/src/drivers/ios/index.ts index e277cc6..19962ff 100644 --- a/src/drivers/ios/index.ts +++ b/src/drivers/ios/index.ts @@ -25,6 +25,16 @@ import type { const COMMAND_TIMEOUT_MS = 30_000; const BOOTSTATUS_TIMEOUT_MS = 120_000; const PROVISION_ESTIMATE_MS = 500; +// Mirrors `downloads.timeoutMs`'s config default (`src/core/config.ts`) -- used only when a +// caller constructs the driver directly without threading the configured value through (tests, +// `SIMLOCK_DRIVERS_MODULE`). +const DEFAULT_DOWNLOAD_TIMEOUT_MS = 1_200_000; +// `simctl`'s `minRuntimeVersion` / `maxRuntimeVersion` encode "no bound" as 0xFFFFFF +// (255.255.255) rather than omitting the field. +const UNBOUNDED_VERSION = 0xff_ff_ff; +// `xcodebuild -downloadPlatform iOS -buildVersion` only reaches back to iOS 16.0 (Xcode +// 16.1+); older runtimes must be installed through Xcode itself. +const IOS_DOWNLOAD_FLOOR: readonly [number, number, number] = [16, 0, 0]; // A cold `simctl boot` to `bootstatus` measures roughly 30s on a fast, idle machine and up to // a minute on a loaded or slower one. The upper end is the estimate, deliberately: this number // is what a waiting requester is quoted, and quoting 30s to someone who then waits 60s is the @@ -46,6 +56,8 @@ interface IosDriverData { export interface IosSimctlDriverOptions { readonly clock: Clock; + /** Per-download timeout; defaults to `downloads.timeoutMs`'s own default. */ + readonly downloadTimeoutMs?: number; readonly filesystem: Filesystem; readonly idGenerator: IdGenerator; readonly processRunner: ProcessRunner; @@ -54,6 +66,10 @@ export interface IosSimctlDriverOptions { interface DeviceType { readonly identifier: string; readonly name: string; + /** Decoded `0xAABBCC` -> `[AA, BB, CC]`; simctl's inclusive lower bound on pairable runtimes. */ + readonly minRuntimeVersion: number; + /** Same encoding; `UNBOUNDED_VERSION` means "no upper bound". */ + readonly maxRuntimeVersion: number; } interface Runtime { @@ -61,6 +77,8 @@ interface Runtime { readonly name: string; readonly version: string; readonly isAvailable: boolean; + /** Device type identifiers this runtime pairs with -- authoritative once the runtime is installed. */ + readonly supportedDeviceTypeIds: ReadonlySet; } interface SimctlCatalog { @@ -82,6 +100,8 @@ type ProcessOutcome = export class IosSimctlDriver implements Driver { readonly platform = "ios" as const; readonly #clock: Clock; + readonly #downloadLocks = new Map>(); + readonly #downloadTimeoutMs: number; readonly #filesystem: Filesystem; readonly #idGenerator: IdGenerator; readonly #processRunner: ProcessRunner; @@ -90,6 +110,7 @@ export class IosSimctlDriver implements Driver { constructor(options: IosSimctlDriverOptions) { this.#clock = options.clock; + this.#downloadTimeoutMs = options.downloadTimeoutMs ?? DEFAULT_DOWNLOAD_TIMEOUT_MS; this.#filesystem = options.filesystem; this.#idGenerator = options.idGenerator; this.#processRunner = options.processRunner; @@ -97,7 +118,7 @@ export class IosSimctlDriver implements Driver { async resolveSpec( request: DeviceRequest, - _options: { readonly allowDownload: boolean }, + options: { readonly allowDownload: boolean }, ): Promise { this.#requireIosPlatform(request.platform); const catalog = await this.#loadCatalog(); @@ -106,19 +127,112 @@ export class IosSimctlDriver implements Driver { ); if (deviceType === undefined) { - throw new UnknownModelError(this.platform, request.model); + throw new IosUnknownModelError(request.model); } - const installedRuntimes = catalog.runtimes.filter((runtime) => runtime.isAvailable); - const runtime = - request.osVersion === undefined - ? newestRuntime(installedRuntimes) - : installedRuntimes.find((candidate) => candidate.version === request.osVersion); + return request.osVersion === undefined + ? this.#resolveDefaultRuntime(deviceType, catalog, options) + : this.#resolveExactRuntime(deviceType, request.osVersion, catalog, options); + } + + /** + * Version requested explicitly: validated against the model's `[min, max]` pairing range + * *before* anything else -- an out-of-range request can never be fixed by downloading, so it + * must never even reach the download decision. + */ + async #resolveExactRuntime( + deviceType: DeviceType, + osVersion: string, + catalog: SimctlCatalog, + options: { readonly allowDownload: boolean }, + ): Promise { + if (!isVersionInRange(osVersion, deviceType)) { + throw new IosVersionOutOfRangeError(deviceType.name, osVersion, deviceType); + } + + const installed = findInstalledRuntime(catalog, osVersion); + if (installed !== undefined) { + return this.#commitResolution(deviceType, installed); + } + + if (!options.allowDownload) { + throw new IosRuntimeMissingError( + osVersion, + `iOS ${osVersion} is not installed; pass --allow-download (or set downloads.policy) ` + + `to download it`, + ); + } + + if (isTooOldToDownload(osVersion)) { + throw new DriverCrashError( + `iOS ${osVersion} predates Xcode's automatic download support (introduced for iOS ` + + `16.0 and newer); install it manually via Xcode`, + ); + } + + await this.#downloadRuntime(["-downloadPlatform", "iOS", "-buildVersion", osVersion]); + const refreshed = await this.#loadCatalog(); + const runtime = findInstalledRuntime(refreshed, osVersion); + if (runtime === undefined) { + throw new DriverCrashError( + `xcodebuild reported success but iOS ${osVersion} is still not installed`, + ); + } + return this.#commitResolution(deviceType, runtime); + } + + /** + * No version requested: defaults to the newest *installed* runtime that both falls in the + * model's range and actually pairs with it (`supportedDeviceTypes`) -- not the newest + * installed runtime overall, which may have dropped this model (iOS 26 dropping iPhone + * XS/XR support is the motivating case). + */ + async #resolveDefaultRuntime( + deviceType: DeviceType, + catalog: SimctlCatalog, + options: { readonly allowDownload: boolean }, + ): Promise { + const paired = pairedInstalledRuntime(catalog, deviceType); + if (paired !== undefined) { + return this.#commitResolution(deviceType, paired); + } + + if (!options.allowDownload) { + throw new IosRuntimeMissingError( + "default", + `No installed iOS runtime pairs with ${deviceType.name}; pass --allow-download (or ` + + `set downloads.policy) to download a compatible runtime`, + ); + } + + if (isUnboundedMax(deviceType.maxRuntimeVersion)) { + // No upper bound on this model's pairing range: any released version works, so there is + // nothing more specific to ask for than "latest". + await this.#downloadRuntime(["-downloadPlatform", "iOS"]); + } else { + const major = majorVersionString(deviceType.maxRuntimeVersion); + try { + await this.#downloadRuntime(["-downloadPlatform", "iOS", "-buildVersion", major]); + } catch (error: unknown) { + throw new DriverCrashError( + `Could not download a default iOS runtime for ${deviceType.name} (tried ${major}): ` + + `${errorMessage(error)}; pass --os to request an exact release`, + ); + } + } + const refreshed = await this.#loadCatalog(); + const runtime = pairedInstalledRuntime(refreshed, deviceType); if (runtime === undefined) { - throw new IosRuntimeMissingError(request.osVersion ?? "default"); + throw new DriverCrashError( + `xcodebuild reported success but no installed iOS runtime pairs with ` + + `${deviceType.name} yet`, + ); } + return this.#commitResolution(deviceType, runtime); + } + #commitResolution(deviceType: DeviceType, runtime: Runtime): DeviceSpec { const spec: DeviceSpec = { model: deviceType.name, osVersion: runtime.version, @@ -128,6 +242,40 @@ export class IosSimctlDriver implements Driver { return spec; } + /** + * Runs `xcodebuild -downloadPlatform iOS [-buildVersion ]`, deduping concurrent + * callers that ask for the exact same invocation behind one in-flight promise -- mirrors the + * Android driver's `#locks` pattern, sized to a single component instead of a whole device. + * The map entry is removed once the download settles (success or failure), so a later, + * non-concurrent call starts a fresh attempt rather than replaying a stale result. + */ + async #downloadRuntime(args: readonly string[]): Promise { + const key = args.join(""); + const inFlight = this.#downloadLocks.get(key); + if (inFlight !== undefined) { + return inFlight; + } + + const promise = this.#xcodebuildOrThrow(args).finally(() => { + if (this.#downloadLocks.get(key) === promise) { + this.#downloadLocks.delete(key); + } + }); + this.#downloadLocks.set(key, promise); + return promise; + } + + async #xcodebuildOrThrow(args: readonly string[]): Promise { + const result = await this.#processRunner.run("xcodebuild", args, { + timeoutMs: this.#downloadTimeoutMs, + }); + if (result.code !== 0) { + throw new DriverCrashError( + `xcodebuild ${args.join(" ")} failed: ${result.stderr || result.stdout}`, + ); + } + } + async provision(spec: DeviceSpec): Promise { this.#requireIosPlatform(spec.platform); const resolved = await this.#resolvedSpec(spec); @@ -467,9 +615,32 @@ export class IosSimctlDriver implements Driver { } class IosRuntimeMissingError extends RuntimeMissingError { - constructor(osVersion: string) { + constructor(osVersion: string, message: string) { super("ios", osVersion); - this.message = `iOS runtime ${osVersion} is not installed; install it via Xcode`; + this.message = message; + } +} + +class IosUnknownModelError extends UnknownModelError { + constructor(model: string) { + super("ios", model); + this.message = `Unknown ios model: ${model}; a newer Xcode version may add this device`; + } +} + +/** + * A requested OS version outside the model's `[minRuntimeVersion, maxRuntimeVersion]` pairing + * range. Extends `RuntimeMissingError` (rather than living as an unrelated class) so it flows + * through the daemon/CLI exactly like `IosRuntimeMissingError` already does -- same error code, + * same exit status -- without either needing to learn a new type. Unlike a missing runtime, no + * download can ever fix this, which is why the message states the supported range instead of + * pointing at `--allow-download`. + */ +class IosVersionOutOfRangeError extends RuntimeMissingError { + constructor(model: string, requested: string, deviceType: DeviceType) { + super("ios", requested); + const range = formatVersionRange(deviceType.minRuntimeVersion, deviceType.maxRuntimeVersion); + this.message = `${model} supports iOS ${range}; iOS ${requested} is out of range`; } } @@ -539,7 +710,14 @@ function parseDeviceType(value: unknown): readonly DeviceType[] { return []; } - return [{ identifier: value.identifier, name: value.name }]; + return [ + { + identifier: value.identifier, + maxRuntimeVersion: versionIntOr(value.maxRuntimeVersion, UNBOUNDED_VERSION), + minRuntimeVersion: versionIntOr(value.minRuntimeVersion, 0), + name: value.name, + }, + ]; } function parseRuntime(value: unknown): readonly Runtime[] { @@ -559,11 +737,89 @@ function parseRuntime(value: unknown): readonly Runtime[] { identifier: value.identifier, isAvailable: value.isAvailable, name: value.name, + supportedDeviceTypeIds: parseSupportedDeviceTypeIds(value.supportedDeviceTypes), version: value.version, }, ]; } +function versionIntOr(value: unknown, fallback: number): number { + return typeof value === "number" && Number.isFinite(value) ? value : fallback; +} + +function parseSupportedDeviceTypeIds(value: unknown): ReadonlySet { + if (!Array.isArray(value)) { + return new Set(); + } + const ids = value + .filter(isRecord) + .map((entry) => entry.identifier) + .filter((identifier): identifier is string => typeof identifier === "string"); + return new Set(ids); +} + +function findInstalledRuntime(catalog: SimctlCatalog, version: string): Runtime | undefined { + return catalog.runtimes.find((runtime) => runtime.isAvailable && runtime.version === version); +} + +/** Installed, in the model's range, and pairs with it -- the newest of those, or none. */ +function pairedInstalledRuntime( + catalog: SimctlCatalog, + deviceType: DeviceType, +): Runtime | undefined { + const candidates = catalog.runtimes.filter( + (runtime) => + runtime.isAvailable && + runtime.supportedDeviceTypeIds.has(deviceType.identifier) && + isVersionInRange(runtime.version, deviceType), + ); + return newestRuntime(candidates); +} + +/** `simctl`'s `0xAABBCC` encoding -> `[major, minor, patch]`. */ +function decodeVersionTriple(encoded: number): readonly [number, number, number] { + return [(encoded >> 16) & 0xff, (encoded >> 8) & 0xff, encoded & 0xff]; +} + +function formatDecodedVersion(encoded: number): string { + const [major, minor, patch] = decodeVersionTriple(encoded); + return patch === 0 ? `${major}.${minor}` : `${major}.${minor}.${patch}`; +} + +function isUnboundedMax(encoded: number): boolean { + return encoded >= UNBOUNDED_VERSION; +} + +function formatVersionRange(min: number, max: number): string { + const minLabel = formatDecodedVersion(min); + return isUnboundedMax(max) ? `${minLabel}+` : `${minLabel}-${formatDecodedVersion(max)}`; +} + +function versionTriple(version: string): readonly [number, number, number] { + const parts = version.split(".").map(versionPart); + return [parts[0] ?? 0, parts[1] ?? 0, parts[2] ?? 0]; +} + +function versionOrdinal(triple: readonly [number, number, number]): number { + return triple[0] * 1_000_000 + triple[1] * 1_000 + triple[2]; +} + +function isVersionInRange(version: string, deviceType: DeviceType): boolean { + const ordinal = versionOrdinal(versionTriple(version)); + return ( + ordinal >= versionOrdinal(decodeVersionTriple(deviceType.minRuntimeVersion)) && + ordinal <= versionOrdinal(decodeVersionTriple(deviceType.maxRuntimeVersion)) + ); +} + +function isTooOldToDownload(version: string): boolean { + return versionOrdinal(versionTriple(version)) < versionOrdinal(IOS_DOWNLOAD_FLOOR); +} + +function majorVersionString(encoded: number): string { + return String(decodeVersionTriple(encoded)[0]); +} + function newestRuntime(runtimes: readonly Runtime[]): Runtime | undefined { return [...runtimes].sort((left, right) => compareVersions(left.version, right.version)).at(-1); } From a6fd4d67f08342e1cf198a1a237c3d304e70979d Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Tue, 1 Sep 2026 16:11:40 +0200 Subject: [PATCH 3/6] feat(android): pluggable read-only device-profile sources and license-gated installs (#67) Refactors avdmanager's built-in device-profile resolution behind a DeviceProfileSource port (ordered list, first match wins) and adds a second source that does a read-only, dependency-free parse of Android Studio's ~/.android/devices.xml, mapping its hardware fields onto the config.ini properties a simlock-created AVD needs. A devices.xml-only profile is applied to config.ini right after `avdmanager create avd`, before the driver's snapshot/config-hash baseline is captured. Also wires downloads.acceptAndroidLicenses through to the driver: an unaccepted-license install failure is detected from sdkmanager's own output and either fails naming the config key, or accepts licenses via piped stdin confirmation and retries the install once. Replaces the hardcoded SDK_DOWNLOAD_TIMEOUT_MS with a downloadTimeoutMs option wired from downloads.timeoutMs in the daemon, mirroring the iOS driver. --- src/daemon/main.ts | 13 +- .../android/device-profile-source.test.ts | 262 ++++++++++++ src/drivers/android/device-profile-source.ts | 404 ++++++++++++++++++ src/drivers/android/index.test.ts | 316 +++++++++++++- src/drivers/android/index.ts | 248 ++++++++--- src/ports/process-runner.ts | 10 + 6 files changed, 1196 insertions(+), 57 deletions(-) create mode 100644 src/drivers/android/device-profile-source.test.ts create mode 100644 src/drivers/android/device-profile-source.ts diff --git a/src/daemon/main.ts b/src/daemon/main.ts index 1514a22..b718f40 100644 --- a/src/daemon/main.ts +++ b/src/daemon/main.ts @@ -89,6 +89,7 @@ export async function startDaemon(options: StartDaemonOptions = {}): Promise { + it("resolves by name or id, case-insensitively", async () => { + const runner = new ScriptedProcessRunner([ + processResult(pixelDevices), + processResult(pixelDevices), + processResult(pixelDevices), + ]); + const source = new BuiltinDeviceProfileSource(avdmanager, runner); + + await expect(source.resolve("Pixel 8")).resolves.toEqual({ + avdmanagerId: "pixel_8", + kind: "builtin", + name: "Pixel 8", + }); + await expect(source.resolve("PIXEL_8")).resolves.toEqual({ + avdmanagerId: "pixel_8", + kind: "builtin", + name: "Pixel 8", + }); + await expect(source.resolve("Pixel Fold")).resolves.toBeUndefined(); + }); + + it("lists resolvable model names", async () => { + const runner = new ScriptedProcessRunner([processResult(pixelDevices)]); + const source = new BuiltinDeviceProfileSource(avdmanager, runner); + + await expect(source.listModels()).resolves.toEqual(["Pixel 8"]); + }); +}); + +describe("UserDeviceProfileSource", () => { + it("resolves a properties profile mapped from devices.xml hardware fields", async () => { + const filesystem = await filesystemWithDevicesXml(devicesXml()); + const source = new UserDeviceProfileSource(devicesXmlPath, filesystem); + + await expect(source.resolve("My Custom Phone")).resolves.toEqual({ + hardwareProperties: { + "hw.device.manufacturer": "Acme", + "hw.device.name": "My Custom Phone", + "hw.lcd.density": "420", + "hw.lcd.height": "2400", + "hw.lcd.width": "1080", + "hw.ramSize": "6144", + }, + kind: "properties", + name: "My Custom Phone", + }); + }); + + it("resolves nothing for a model it does not have", async () => { + const filesystem = await filesystemWithDevicesXml(devicesXml()); + const source = new UserDeviceProfileSource(devicesXmlPath, filesystem); + + await expect(source.resolve("Pixel 8")).resolves.toBeUndefined(); + }); + + it("treats an absent file as no profiles without a diagnostic", async () => { + const filesystem = new MemoryFilesystem(); + const diagnostics: DeviceProfileSourceDiagnostic[] = []; + const source = new UserDeviceProfileSource(devicesXmlPath, filesystem, (diagnostic) => + diagnostics.push(diagnostic), + ); + + await expect(source.listModels()).resolves.toEqual([]); + expect(diagnostics).toEqual([]); + }); + + it("reports malformed devices.xml as a diagnostic instead of throwing", async () => { + const filesystem = await filesystemWithDevicesXml("not even close to xml {{{"); + const diagnostics: DeviceProfileSourceDiagnostic[] = []; + const source = new UserDeviceProfileSource(devicesXmlPath, filesystem, (diagnostic) => + diagnostics.push(diagnostic), + ); + + await expect(source.listModels()).resolves.toEqual([]); + await expect(source.resolve("anything")).resolves.toBeUndefined(); + expect(diagnostics).toHaveLength(2); + expect(diagnostics[0]).toMatchObject({ + kind: "device-profile-source-unreadable", + path: devicesXmlPath, + }); + }); + + it("treats a well-formed but empty devices.xml as legitimately profile-less", async () => { + const filesystem = await filesystemWithDevicesXml( + '', + ); + const diagnostics: DeviceProfileSourceDiagnostic[] = []; + const source = new UserDeviceProfileSource(devicesXmlPath, filesystem, (diagnostic) => + diagnostics.push(diagnostic), + ); + + await expect(source.listModels()).resolves.toEqual([]); + expect(diagnostics).toEqual([]); + }); +}); + +describe("parseDevicesXml", () => { + it("maps named density buckets and KiB ram to config.ini-shaped values", () => { + const xml = ` + + + Bucket Phone + + + normal + xxhdpi + + 1440 + 3040 + + + + 4194304 + + + + `; + + expect(parseDevicesXml(xml)).toEqual([ + { + hardwareProperties: { + "hw.device.name": "Bucket Phone", + "hw.lcd.density": "480", + "hw.lcd.height": "3040", + "hw.lcd.width": "1440", + "hw.ramSize": "4096", + }, + name: "Bucket Phone", + }, + ]); + }); + + it("skips a device with no name", () => { + const xml = ` + 2048 + `; + + expect(parseDevicesXml(xml)).toEqual([]); + }); + + it("returns no profiles for an empty file without throwing", () => { + expect(parseDevicesXml("")).toEqual([]); + expect(parseDevicesXml(" \n ")).toEqual([]); + }); + + it("throws for content with no recognizable root", () => { + expect(() => parseDevicesXml("")).toThrow(); + expect(() => parseDevicesXml("this is not xml")).toThrow(); + }); +}); + +describe("DeviceProfileRegistry", () => { + it("resolves the first source's profile when two sources both name the same model", async () => { + const runner = new ScriptedProcessRunner([processResult(pixelDevices)]); + const builtin = new BuiltinDeviceProfileSource(avdmanager, runner); + const filesystem = await filesystemWithDevicesXml( + devicesXml().replace("My Custom Phone", "Pixel 8"), + ); + const user = new UserDeviceProfileSource(devicesXmlPath, filesystem); + const registry = new DeviceProfileRegistry([builtin, user]); + + await expect(registry.resolve("Pixel 8")).resolves.toEqual({ + avdmanagerId: "pixel_8", + kind: "builtin", + name: "Pixel 8", + }); + }); + + it("falls through to a later source when the first has no match", async () => { + const runner = new ScriptedProcessRunner([processResult(pixelDevices)]); + const builtin = new BuiltinDeviceProfileSource(avdmanager, runner); + const filesystem = await filesystemWithDevicesXml(devicesXml()); + const user = new UserDeviceProfileSource(devicesXmlPath, filesystem); + const registry = new DeviceProfileRegistry([builtin, user]); + + await expect(registry.resolve("My Custom Phone")).resolves.toEqual({ + hardwareProperties: { + "hw.device.manufacturer": "Acme", + "hw.device.name": "My Custom Phone", + "hw.lcd.density": "420", + "hw.lcd.height": "2400", + "hw.lcd.width": "1080", + "hw.ramSize": "6144", + }, + kind: "properties", + name: "My Custom Phone", + }); + }); + + it("rejects an unresolvable model with UnknownModelError", async () => { + const runner = new ScriptedProcessRunner([processResult(pixelDevices)]); + const builtin = new BuiltinDeviceProfileSource(avdmanager, runner); + const registry = new DeviceProfileRegistry([builtin]); + + await expect(registry.resolve("Nope")).rejects.toMatchObject({ name: "UnknownModelError" }); + }); + + it("dedupes listModels by name, earliest source winning", async () => { + const runner = new ScriptedProcessRunner([processResult(pixelDevices)]); + const builtin = new BuiltinDeviceProfileSource(avdmanager, runner); + const filesystem = await filesystemWithDevicesXml( + devicesXml().replace("My Custom Phone", "pixel 8"), + ); + const user = new UserDeviceProfileSource(devicesXmlPath, filesystem); + const registry = new DeviceProfileRegistry([builtin, user]); + + await expect(registry.listModels()).resolves.toEqual(["Pixel 8"]); + }); +}); + +function devicesXml(): string { + return ` + + + My Custom Phone + Acme + + + normal + 420dpi + + 1080 + 2400 + + + + 6144 + + + + `; +} + +async function filesystemWithDevicesXml(contents: string): Promise { + const filesystem = new MemoryFilesystem(); + await filesystem.mkdirp("/home/simlock/.android"); + await filesystem.writeFileAtomic(devicesXmlPath, contents); + return filesystem; +} + +function processResult(stdout: string) { + return { + match: { args: ["list", "device"], command: avdmanager }, + result: { code: 0, stderr: "", stdout }, + }; +} diff --git a/src/drivers/android/device-profile-source.ts b/src/drivers/android/device-profile-source.ts new file mode 100644 index 0000000..2f92811 --- /dev/null +++ b/src/drivers/android/device-profile-source.ts @@ -0,0 +1,404 @@ +import { DriverCrashError, UnknownModelError } from "../../core/driver.js"; +import type { Filesystem, ProcessRunner } from "../../ports/index.js"; + +/** + * What `DeviceProfileSource#resolve` hands back for a model name. `builtin` is today's + * `avdmanager -d ` path; `properties` is a hardware descriptor applied to `config.ini` + * after `avdmanager create avd` (see `AndroidDriver#provision`) -- there is no `avdmanager` + * device id for it because it never came from `avdmanager list device`. `name` is the + * source's own canonical spelling of the model (case may differ from what the caller asked + * for), which `AndroidDriver` needs so `DeviceSpec.model` and cache lookups stay stable. + */ +export type ResolvedDeviceProfile = + | { readonly kind: "builtin"; readonly name: string; readonly avdmanagerId: string } + | { + readonly kind: "properties"; + readonly name: string; + readonly hardwareProperties: Readonly>; + }; + +/** + * Diagnostics a `DeviceProfileSource` can raise. Sources never throw out of `resolve` / + * `listModels` for a data problem (an absent, unreadable, or malformed file is not the + * caller's fault) -- this is the only channel for surfacing that something was ignored. + */ +export interface DeviceProfileSourceDiagnostic { + readonly kind: "device-profile-source-unreadable"; + readonly path: string; + readonly reason: string; +} + +/** + * A read-only place `AndroidDriver` can load device profiles from. Simlock never writes to + * any of these locations -- see safety rule 1 -- sources only load. Implementations must + * never throw for a missing or malformed backing store; report that through the + * `onDiagnostic` callback they were constructed with instead, and resolve to "nothing here." + */ +export interface DeviceProfileSource { + /** Resolvable model names this source can currently answer for. */ + listModels(): Promise; + /** `undefined` when this source has no profile for `model` -- never throws for a miss. */ + resolve(model: string): Promise; +} + +/** + * Ordered list of sources, first match wins. This is the whole extension point: a future + * community/network source is a new `DeviceProfileSource` implementation plus one more entry + * in the list a driver is constructed with -- nothing else in the driver changes. + */ +export class DeviceProfileRegistry { + readonly #sources: readonly DeviceProfileSource[]; + + constructor(sources: readonly DeviceProfileSource[]) { + this.#sources = sources; + } + + /** Dedupes by name, case-insensitively; the earliest source in the list wins a collision. */ + async listModels(): Promise { + const seen = new Map(); + for (const source of this.#sources) { + for (const name of await source.listModels()) { + const key = name.toLocaleLowerCase(); + if (!seen.has(key)) { + seen.set(key, name); + } + } + } + return [...seen.values()]; + } + + async resolve(model: string): Promise { + for (const source of this.#sources) { + const resolved = await source.resolve(model); + if (resolved !== undefined) { + return resolved; + } + } + throw new UnknownModelError("android", model); + } +} + +/** Today's behavior, refactored behind `DeviceProfileSource`: resolves against `avdmanager list device`. */ +export class BuiltinDeviceProfileSource implements DeviceProfileSource { + readonly #avdmanager: string; + readonly #processRunner: ProcessRunner; + + constructor(avdmanager: string, processRunner: ProcessRunner) { + this.#avdmanager = avdmanager; + this.#processRunner = processRunner; + } + + // fallow-ignore-next-line unused-class-member -- reached through the DeviceProfileSource port by DeviceProfileRegistry.listModels. + async listModels(): Promise { + return (await this.#profiles()).map((profile) => profile.name); + } + + async resolve(model: string): Promise { + const normalized = model.toLocaleLowerCase(); + const profile = (await this.#profiles()).find( + (candidate) => + candidate.name.toLocaleLowerCase() === normalized || + candidate.id.toLocaleLowerCase() === normalized, + ); + return profile === undefined + ? undefined + : { avdmanagerId: profile.id, kind: "builtin", name: profile.name }; + } + + async #profiles(): Promise { + const result = await this.#processRunner.run(this.#avdmanager, ["list", "device"]); + if (result.code !== 0) { + throw new DriverCrashError( + `${this.#avdmanager} list device failed: ${result.stderr || result.stdout}`, + ); + } + return parseAvdmanagerDeviceProfiles(result.stdout); + } +} + +interface AvdmanagerDeviceProfile { + readonly id: string; + readonly name: string; +} + +export function parseAvdmanagerDeviceProfiles(output: string): AvdmanagerDeviceProfile[] { + const profiles: AvdmanagerDeviceProfile[] = []; + let id: string | undefined; + for (const line of output.split(/\r?\n/)) { + const idMatch = /^id:\s*\d+\s+or\s+"([^"]+)"/.exec(line.trim()); + if (idMatch?.[1] !== undefined) { + id = idMatch[1]; + continue; + } + const nameMatch = /^Name:\s*(.+)$/.exec(line.trim()); + if (id !== undefined && nameMatch?.[1] !== undefined) { + profiles.push({ id, name: nameMatch[1] }); + id = undefined; + } + } + return profiles; +} + +/** + * Read-only parse of Android Studio's `~/.android/devices.xml` -- that file belongs to + * Android Studio; Simlock only ever reads it. Resolved profiles carry hardware properties + * (screen resolution, density, RAM) applied to the simlock-created AVD's `config.ini`, since + * there is no official profile store beyond `avdmanager`'s own built-ins to create the AVD + * from directly. + * + * Parsed without a runtime dependency: a minimal, tolerant tag extractor rather than a real + * XML parser (see `parseDevicesXml` below for exactly what it reads and what it deliberately + * ignores). + */ +export class UserDeviceProfileSource implements DeviceProfileSource { + readonly #filesystem: Filesystem; + readonly #onDiagnostic: ((diagnostic: DeviceProfileSourceDiagnostic) => void) | undefined; + readonly #path: string; + + constructor( + path: string, + filesystem: Filesystem, + onDiagnostic?: (diagnostic: DeviceProfileSourceDiagnostic) => void, + ) { + this.#filesystem = filesystem; + this.#onDiagnostic = onDiagnostic; + this.#path = path; + } + + async listModels(): Promise { + return (await this.#profiles()).map((profile) => profile.name); + } + + async resolve(model: string): Promise { + const normalized = model.toLocaleLowerCase(); + const profile = (await this.#profiles()).find( + (candidate) => candidate.name.toLocaleLowerCase() === normalized, + ); + return profile === undefined + ? undefined + : { hardwareProperties: profile.hardwareProperties, kind: "properties", name: profile.name }; + } + + async #profiles(): Promise { + if (!(await this.#filesystem.exists(this.#path))) { + // Android Studio never having run, or never having any custom profiles, is the common + // case, not a diagnostic-worthy one. + return []; + } + + let contents: string; + try { + contents = await this.#filesystem.readFile(this.#path); + } catch (error: unknown) { + this.#reportUnreadable(errorMessage(error)); + return []; + } + + try { + return parseDevicesXml(contents); + } catch (error: unknown) { + this.#reportUnreadable(errorMessage(error)); + return []; + } + } + + #reportUnreadable(reason: string): void { + this.#onDiagnostic?.({ kind: "device-profile-source-unreadable", path: this.#path, reason }); + } +} + +interface DevicesXmlProfile { + readonly name: string; + readonly hardwareProperties: Readonly>; +} + +/** + * Maps a `` element to the `config.ini` hardware properties `avdmanager -d` would + * otherwise have produced. Mapped: + * + * - `` -> `hw.device.name` + * - `` -> `hw.device.manufacturer` (when present) + * - `` / `` -> `hw.lcd.width` / `hw.lcd.height` + * - `` -> `hw.lcd.density` (bucket name or `dpi` -> numeric dpi) + * - `` -> `hw.ramSize` (converted to MiB, unit-suffix-free like avdmanager writes it) + * + * Deliberately skipped, because `config.ini`'s AVD-identity hash and the emulator's own + * defaults cover them well enough that mirroring them exactly is not worth the parsing + * surface: `` / `` / `` (physical form + * factor, not a rendering input), `` / `` (physical DPI, distinct from the + * rendering `pixel-density` bucket this already maps), ``, ``, + * ``, ``, ``, ``, ``, `` (the system image + * the caller picked already determines the ABI), ``, and the whole `` + * block (API-level / feature compatibility simlock resolves separately via the system image). + * A device with multiple `` hardware variants (e.g. a foldable's postures) is read + * from whichever `` block appears first in the file, matching `` / + * `` textually rather than per-state. + */ +export function parseDevicesXml(contents: string): readonly DevicesXmlProfile[] { + const trimmed = contents.trim(); + if (trimmed === "") { + return []; + } + if (!/<([\w.-]+:)?devices[\s/>]/i.test(trimmed)) { + throw new Error("devices.xml has no recognizable root element"); + } + + const profiles: DevicesXmlProfile[] = []; + for (const block of extractElements(trimmed, "device")) { + const rawName = extractText(block, "name"); + if (rawName === undefined || rawName === "") { + continue; + } + const name = unescapeXml(rawName); + const hardwareProperties: Record = { "hw.device.name": name }; + + const manufacturer = extractText(block, "manufacturer"); + if (manufacturer !== undefined && manufacturer !== "") { + hardwareProperties["hw.device.manufacturer"] = unescapeXml(manufacturer); + } + + applyScreenProperties(block, hardwareProperties); + applyRamProperty(block, hardwareProperties); + + profiles.push({ hardwareProperties, name }); + } + return profiles; +} + +function applyScreenProperties( + deviceBlock: string, + hardwareProperties: Record, +): void { + const screen = extractText(deviceBlock, "screen"); + if (screen === undefined) { + return; + } + + const dimensions = extractText(screen, "dimensions"); + if (dimensions !== undefined) { + const width = extractText(dimensions, "x-dimension"); + const height = extractText(dimensions, "y-dimension"); + if (width !== undefined && /^\d+$/.test(width)) { + hardwareProperties["hw.lcd.width"] = width; + } + if (height !== undefined && /^\d+$/.test(height)) { + hardwareProperties["hw.lcd.height"] = height; + } + } + + const density = extractText(screen, "pixel-density"); + if (density !== undefined) { + const dpi = densityToDpi(density); + if (dpi !== undefined) { + hardwareProperties["hw.lcd.density"] = String(dpi); + } + } +} + +function applyRamProperty(deviceBlock: string, hardwareProperties: Record): void { + const ram = extractElement(deviceBlock, "ram-size"); + if (ram === undefined) { + return; + } + const megabytes = ramSizeToMebibytes(ram.text, ram.attributes["unit"]); + if (megabytes !== undefined) { + hardwareProperties["hw.ramSize"] = String(megabytes); + } +} + +/** Android's standard density buckets, matching what `avdmanager`'s own built-ins resolve to. */ +const DENSITY_BUCKETS_DPI: Readonly> = { + ldpi: 120, + mdpi: 160, + tvdpi: 213, + hdpi: 240, + xhdpi: 320, + xxhdpi: 480, + xxxhdpi: 640, +}; + +function densityToDpi(raw: string): number | undefined { + const key = raw.trim().toLowerCase(); + const bucket = DENSITY_BUCKETS_DPI[key]; + if (bucket !== undefined) { + return bucket; + } + const numeric = /^(\d+)(dpi)?$/.exec(key); + return numeric?.[1] === undefined ? undefined : Number(numeric[1]); +} + +function ramSizeToMebibytes(text: string, unit: string | undefined): number | undefined { + const value = Number(text.trim()); + if (!Number.isFinite(value)) { + return undefined; + } + switch ((unit ?? "MiB").trim().toUpperCase()) { + case "KIB": + return Math.round(value / 1024); + case "GIB": + return Math.round(value * 1024); + case "TIB": + return Math.round(value * 1024 * 1024); + default: + return Math.round(value); + } +} + +/** All top-level `<(ns:)tag>...` blocks' inner content, in document order. */ +function extractElements(xml: string, tag: string): string[] { + const pattern = new RegExp( + `<(?:[\\w.-]+:)?${tag}(?:\\s[^>]*)?>([\\s\\S]*?)`, + "gi", + ); + const blocks: string[] = []; + for (const match of xml.matchAll(pattern)) { + if (match[1] !== undefined) { + blocks.push(match[1]); + } + } + return blocks; +} + +/** The first `<(ns:)tag>...` block's inner content, or undefined. */ +function extractText(xml: string, tag: string): string | undefined { + return extractElement(xml, tag)?.text; +} + +/** The first `<(ns:)tag attr="...">...` element's attributes and trimmed text. */ +function extractElement( + xml: string, + tag: string, +): { readonly attributes: Readonly>; readonly text: string } | undefined { + const pattern = new RegExp( + `<(?:[\\w.-]+:)?${tag}((?:\\s[^>]*)?)>([\\s\\S]*?)`, + "i", + ); + const match = pattern.exec(xml); + if (match === null) { + return undefined; + } + const attributes: Record = {}; + for (const attributeMatch of (match[1] ?? "").matchAll(/([\w:-]+)\s*=\s*"([^"]*)"/g)) { + const [, key, value] = attributeMatch; + if (key !== undefined && value !== undefined) { + attributes[key] = value; + } + } + return { attributes, text: (match[2] ?? "").trim() }; +} + +const XML_ENTITIES: Readonly> = { + "&": "&", + "'": "'", + ">": ">", + "<": "<", + """: '"', +}; + +function unescapeXml(value: string): string { + return value.replace(/&(?:amp|apos|gt|lt|quot);/g, (entity) => XML_ENTITIES[entity] ?? entity); +} + +function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} diff --git a/src/drivers/android/index.test.ts b/src/drivers/android/index.test.ts index b5b219e..d59fb73 100644 --- a/src/drivers/android/index.test.ts +++ b/src/drivers/android/index.test.ts @@ -11,7 +11,12 @@ import { type ScriptedProcessExpectation, SystemClock, } from "../../ports/index.js"; -import { AndroidDriver, SdkMissingError } from "./index.js"; +import { + AndroidDriver, + AndroidLicenseNotAcceptedError, + SdkMissingError, + type AndroidDriverDiagnostic, +} from "./index.js"; const sdk = "/android-sdk"; const home = "/home/simlock"; @@ -832,6 +837,290 @@ describe("AndroidDriver", () => { const device = reality.devices.find((candidate) => candidate.deviceId === "simlock_legacy"); expect(device?.mark).toBeUndefined(); }); + + describe("device-profile sources", () => { + it("resolves a devices.xml-only model and applies its properties to config.ini before the config hash is captured", async () => { + const filesystem = await androidFilesystem(); + await writeDevicesXml(filesystem, customDeviceXml("Custom A", 4096)); + // `avdmanager create avd` is scripted (not a real process), so it never creates the AVD + // directory the way it would for real -- seed it here the same way, since applying + // hardware properties to config.ini right after create relies on that directory existing. + await filesystem.mkdirp(`${avdDirectory}/simlock_one.avd`); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.avdmanager, [ + "create", + "avd", + "-n", + "simlock_one", + "-k", + /.+/, + "-d", + "pixel_8", + ]), + processResult(binaries.emulator, ["-version"], "Android emulator version 36.1.9"), + processResult(binaries.adb, ["devices"], "List of devices attached\n"), + ]); + const driver = await createDriver(filesystem, runner, { ids: ["one"] }); + + const spec = await driver.resolveSpec( + { model: "Custom A", osVersion: "34", platform: "android" }, + { allowDownload: false }, + ); + expect(spec).toEqual({ model: "Custom A", osVersion: "34", platform: "android" }); + + const device = await driver.provision(spec); + + const config = await filesystem.readFile(`${avdDirectory}/simlock_one.avd/config.ini`); + expect(config).toContain("hw.device.name=Custom A"); + expect(config).toContain("hw.ramSize=4096"); + // The avdmanager `-d` seed uses a built-in device only to skip the interactive prompt -- + // it must never leak into the resolved spec or the applied hardware properties. + expect(config).not.toContain("pixel_8"); + expect(device.driverData).toMatchObject({ avdName: "simlock_one" }); + }); + + it("captures differing hardware properties in the config hash, proving they land before it is computed", async () => { + const buildHarness = async (ramMiB: number) => { + const filesystem = await androidFilesystem(); + await writeDevicesXml(filesystem, customDeviceXml("Custom A", ramMiB)); + await filesystem.mkdirp(`${avdDirectory}/simlock_one.avd`); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.avdmanager, [ + "create", + "avd", + "-n", + "simlock_one", + "-k", + /.+/, + "-d", + "pixel_8", + ]), + processResult(binaries.emulator, ["-version"], "Android emulator version 36.1.9"), + processResult(binaries.adb, ["devices"], "List of devices attached\n"), + ]); + const driver = await createDriver(filesystem, runner, { ids: ["one"] }); + const spec = await driver.resolveSpec( + { model: "Custom A", osVersion: "34", platform: "android" }, + { allowDownload: false }, + ); + return driver.provision(spec); + }; + + const lowRam = await buildHarness(2048); + const highRam = await buildHarness(4096); + + expect((lowRam.driverData as { configHash: string }).configHash).not.toBe( + (highRam.driverData as { configHash: string }).configHash, + ); + }); + + it("shadows a devices.xml profile with a built-in one of the same name and never applies properties", async () => { + const filesystem = await androidFilesystem(); + // Same name as the built-in fixture's "Pixel 8" -- the built-in source is registered + // first, so it must win and the devices.xml properties must never be touched. + await writeDevicesXml(filesystem, customDeviceXml("Pixel 8", 4096)); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.avdmanager, [ + "create", + "avd", + "-n", + "simlock_one", + "-k", + /.+/, + "-d", + "pixel_8", + ]), + processResult(binaries.emulator, ["-version"], "Android emulator version 36.1.9"), + processResult(binaries.adb, ["devices"], "List of devices attached\n"), + ]); + const driver = await createDriver(filesystem, runner, { ids: ["one"] }); + + const spec = await driver.resolveSpec( + { model: "Pixel 8", osVersion: "34", platform: "android" }, + { allowDownload: false }, + ); + await driver.provision(spec); + + // Only one `avdmanager list device` call happened (asserted implicitly by the runner + // never receiving the unscripted second call a `properties`-profile seed lookup would + // require), and no properties were merged into config.ini -- it was never even written. + await expect(filesystem.exists(`${avdDirectory}/simlock_one.avd/config.ini`)).resolves.toBe( + false, + ); + }); + + it("surfaces malformed devices.xml as a diagnostic and falls through to UnknownModelError, never throwing from the parse itself", async () => { + const filesystem = await androidFilesystem(); + await filesystem.mkdirp(`${home}/.android`); + await filesystem.writeFileAtomic(`${home}/.android/devices.xml`, "not xml at all {{{"); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + ]); + const diagnostics: AndroidDriverDiagnostic[] = []; + const driver = await createDriver(filesystem, runner, { + onDiagnostic: (diagnostic) => diagnostics.push(diagnostic), + }); + + await expect( + driver.resolveSpec( + { model: "Nonexistent Model", osVersion: "34", platform: "android" }, + { allowDownload: false }, + ), + ).rejects.toMatchObject({ name: "UnknownModelError" }); + + expect(diagnostics).toEqual([ + expect.objectContaining({ kind: "device-profile-source-unreadable" }), + ]); + }); + }); + + describe("Android SDK license handling", () => { + const licenseNotAcceptedOutput = + "Warning: License for package Android SDK Platform 35 not accepted.\n\n" + + "1 package(s) were skipped due to license issues. Please accept the license(s) and try " + + "again.\nTo resolve, run: sdkmanager --licenses\n"; + + it("fails naming downloads.acceptAndroidLicenses when licenses are unaccepted and the flag is off", async () => { + const filesystem = await androidFilesystem(); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + { + match: { + args: ["--install", "system-images;android-35;google_apis;arm64-v8a"], + command: binaries.sdkmanager, + }, + result: { code: 1, stderr: "", stdout: licenseNotAcceptedOutput }, + }, + ]); + const driver = await createDriver(filesystem, runner, { acceptAndroidLicenses: false }); + + const error = await driver + .resolveSpec( + { model: "Pixel 8", osVersion: "35", platform: "android" }, + { allowDownload: true }, + ) + .catch((caught: unknown) => caught); + + expect(error).toBeInstanceOf(AndroidLicenseNotAcceptedError); + expect((error as Error).message).toContain("downloads.acceptAndroidLicenses"); + expect((error as Error).message).toContain("sdkmanager --licenses"); + }); + + it("accepts licenses through piped confirmation and retries the install once when the flag is on", async () => { + const filesystem = await androidFilesystem(); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + { + match: { + args: ["--install", "system-images;android-35;google_apis;arm64-v8a"], + command: binaries.sdkmanager, + }, + result: { code: 1, stderr: "", stdout: licenseNotAcceptedOutput }, + }, + processResult(binaries.sdkmanager, ["--licenses"], "All licenses accepted.\n"), + processResult(binaries.sdkmanager, [ + "--install", + "system-images;android-35;google_apis;arm64-v8a", + ]), + ]); + const driver = await createDriver(filesystem, runner, { acceptAndroidLicenses: true }); + + await expect( + driver.resolveSpec( + { model: "Pixel 8", osVersion: "35", platform: "android" }, + { allowDownload: true }, + ), + ).resolves.toEqual({ model: "Pixel 8", osVersion: "35", platform: "android" }); + + const licensesCall = runner.calls.find( + (call) => call.command === binaries.sdkmanager && call.args[0] === "--licenses", + ); + expect(licensesCall?.options.input).toBe("y\n".repeat(100)); + // Exactly one retry: install, licenses, install again -- never a second acceptance pass. + expect(runner.calls.filter((call) => call.args[0] === "--install")).toHaveLength(2); + }); + + it("still fails when the install is rejected again after accepting licenses", async () => { + const filesystem = await androidFilesystem(); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + { + match: { + args: ["--install", "system-images;android-35;google_apis;arm64-v8a"], + command: binaries.sdkmanager, + }, + result: { code: 1, stderr: "", stdout: licenseNotAcceptedOutput }, + }, + processResult(binaries.sdkmanager, ["--licenses"], "All licenses accepted.\n"), + { + match: { + args: ["--install", "system-images;android-35;google_apis;arm64-v8a"], + command: binaries.sdkmanager, + }, + result: { code: 1, stderr: "still refusing", stdout: "" }, + }, + ]); + const driver = await createDriver(filesystem, runner, { acceptAndroidLicenses: true }); + + await expect( + driver.resolveSpec( + { model: "Pixel 8", osVersion: "35", platform: "android" }, + { allowDownload: true }, + ), + ).rejects.toMatchObject({ name: "DriverCrashError" }); + }); + }); + + describe("download timeout", () => { + it("threads the configured downloadTimeoutMs into the sdkmanager install call", async () => { + const filesystem = await androidFilesystem(); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.sdkmanager, [ + "--install", + "system-images;android-35;google_apis;arm64-v8a", + ]), + ]); + const driver = await createDriver(filesystem, runner, { downloadTimeoutMs: 42_000 }); + + await driver.resolveSpec( + { model: "Pixel 8", osVersion: "35", platform: "android" }, + { allowDownload: true }, + ); + + const installCall = runner.calls.find( + (call) => call.command === binaries.sdkmanager && call.args[0] === "--install", + ); + expect(installCall?.options.timeoutMs).toBe(42_000); + }); + + it("defaults to the same 20-minute timeout as before this option existed", async () => { + const filesystem = await androidFilesystem(); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.sdkmanager, [ + "--install", + "system-images;android-35;google_apis;arm64-v8a", + ]), + ]); + const driver = await createDriver(filesystem, runner); + + await driver.resolveSpec( + { model: "Pixel 8", osVersion: "35", platform: "android" }, + { allowDownload: true }, + ); + + const installCall = runner.calls.find( + (call) => call.command === binaries.sdkmanager && call.args[0] === "--install", + ); + expect(installCall?.options.timeoutMs).toBe(20 * 60_000); + }); + }); }); const live = process.env.SIMLOCK_LIVE_ANDROID === "1" ? it : it.skip; @@ -977,14 +1266,23 @@ async function createDriver( filesystem: Filesystem, processRunner: ScriptedProcessRunner, options: { + readonly acceptAndroidLicenses?: boolean; readonly clock?: FakeClock; + readonly downloadTimeoutMs?: number; readonly ids?: readonly string[]; + readonly onDiagnostic?: (diagnostic: AndroidDriverDiagnostic) => void; readonly readinessTimeoutMs?: number; } = {}, ) { let nextId = 0; return AndroidDriver.create({ + ...(options.acceptAndroidLicenses === undefined + ? {} + : { acceptAndroidLicenses: options.acceptAndroidLicenses }), clock: options.clock ?? new FakeClock(), + ...(options.downloadTimeoutMs === undefined + ? {} + : { downloadTimeoutMs: options.downloadTimeoutMs }), env: { ANDROID_HOME: sdk }, filesystem, homeDirectory: home, @@ -992,6 +1290,7 @@ async function createDriver( idGenerator: { generate: () => options.ids?.[nextId++] ?? `device-${nextId}`, }, + ...(options.onDiagnostic === undefined ? {} : { onDiagnostic: options.onDiagnostic }), ...(options.readinessTimeoutMs === undefined ? {} : { readinessTimeoutMs: options.readinessTimeoutMs }), @@ -999,6 +1298,21 @@ async function createDriver( }); } +async function writeDevicesXml(filesystem: MemoryFilesystem, deviceBodies: string): Promise { + await filesystem.mkdirp(`${home}/.android`); + await filesystem.writeFileAtomic( + `${home}/.android/devices.xml`, + `${deviceBodies}`, + ); +} + +function customDeviceXml(name: string, ramMiB: number): string { + return ( + `${name}` + + `${ramMiB}` + ); +} + async function androidFilesystem( options: { readonly config?: string; diff --git a/src/drivers/android/index.ts b/src/drivers/android/index.ts index b6326e1..97a2932 100644 --- a/src/drivers/android/index.ts +++ b/src/drivers/android/index.ts @@ -12,7 +12,6 @@ import { type ObservedMark, type ReclaimResult, RuntimeMissingError, - UnknownModelError, } from "../../core/driver.js"; import type { Clock, @@ -23,13 +22,31 @@ import type { ProcessRunner, } from "../../ports/index.js"; import { isAndroidDriverData, type AndroidDriverData } from "./data.js"; +import { + BuiltinDeviceProfileSource, + DeviceProfileRegistry, + parseAvdmanagerDeviceProfiles, + UserDeviceProfileSource, + type DeviceProfileSource, + type DeviceProfileSourceDiagnostic, + type ResolvedDeviceProfile, +} from "./device-profile-source.js"; const DEFAULT_READINESS_TIMEOUT_MS = 180_000; const COLD_BOOT_ESTIMATE_MS = 31_000; const PORT_MAX = 5682; const PORT_MIN = 5554; const PORT_POLL_INTERVAL_MS = 2_000; -const SDK_DOWNLOAD_TIMEOUT_MS = 20 * 60_000; +// Mirrors `downloads.timeoutMs`'s config default (`src/core/config.ts`) -- used only when a +// caller constructs the driver directly without threading the configured value through (tests, +// `SIMLOCK_DRIVERS_MODULE`). See the iOS driver's `DEFAULT_DOWNLOAD_TIMEOUT_MS` for the same +// pattern. +const DEFAULT_DOWNLOAD_TIMEOUT_MS = 20 * 60_000; +// `sdkmanager --licenses` prompts once per outstanding license with a bare `y/N`. Answering +// more times than there are real licenses is harmless -- the extra `y`s land after the prompt +// loop has already exited and sdkmanager simply never reads them -- so this just needs to be +// comfortably above the largest real Android SDK license count rather than exact. +const LICENSE_ACCEPT_ANSWERS = 100; // A defense-in-depth bound on the wait that follows a SIGKILL: NodeProcessHandle#wait // already settles shortly after `exit`, but this keeps a pathologically slow reap // from ever turning a "we already killed it" cleanup into an unbounded await. @@ -57,7 +74,21 @@ const DURABLE_MARK_KEY = "simlock.mark"; const ERASABLE_MARK_PATH = "/data/local/tmp/simlock-mark.json"; export interface AndroidDriverOptions { + /** + * Explicit legal consent for Android SDK licenses (`downloads.acceptAndroidLicenses`), + * independent of the per-request download permission. Defaults to `false`: an install that + * fails on an unaccepted license fails outright rather than accepting it silently. + */ + readonly acceptAndroidLicenses?: boolean; readonly clock: Clock; + /** + * Ordered device-profile sources, first match wins (see `DeviceProfileRegistry`). Defaults + * to `[builtin, user]` -- `avdmanager list device` first, then a read-only parse of + * `~/.android/devices.xml`, so a name defined in both resolves to the built-in. + */ + readonly deviceProfileSources?: readonly DeviceProfileSource[]; + /** Per-install timeout for `sdkmanager`; defaults to `downloads.timeoutMs`'s own default. */ + readonly downloadTimeoutMs?: number; readonly env: Readonly>; readonly filesystem: Filesystem; readonly homeDirectory: string; @@ -68,11 +99,9 @@ export interface AndroidDriverOptions { readonly readinessTimeoutMs?: number; } -export interface AndroidDriverDiagnostic { - readonly avdName: string; - readonly kind: "snapshot-cold-boot"; - readonly readyAfterMs: number; -} +export type AndroidDriverDiagnostic = + | { readonly avdName: string; readonly kind: "snapshot-cold-boot"; readonly readyAfterMs: number } + | DeviceProfileSourceDiagnostic; export class SdkMissingError extends Error { constructor(readonly searchedPaths: readonly string[]) { @@ -81,6 +110,17 @@ export class SdkMissingError extends Error { } } +export class AndroidLicenseNotAcceptedError extends Error { + constructor(readonly packageName: string) { + super( + `sdkmanager refused to install ${packageName}: an Android SDK license is not accepted. ` + + `Set "downloads.acceptAndroidLicenses": true in config to accept automatically, or run ` + + `\`sdkmanager --licenses\` manually.`, + ); + this.name = "AndroidLicenseNotAcceptedError"; + } +} + interface AndroidSdkPaths { readonly adb: string; readonly avdmanager: string; @@ -105,17 +145,15 @@ interface SystemImage { readonly version: string; } -interface DeviceProfile { - readonly id: string; - readonly name: string; -} - const allocationsByRunner = new WeakMap(); export class AndroidDriver implements Driver { readonly platform = "android" as const; + readonly #acceptAndroidLicenses: boolean; readonly #clock: Clock; + readonly #deviceProfiles: DeviceProfileRegistry; readonly #devices = new Map(); + readonly #downloadTimeoutMs: number; readonly #filesystem: Filesystem; readonly #hostAbi: string; readonly #idGenerator: IdGenerator; @@ -123,13 +161,15 @@ export class AndroidDriver implements Driver { readonly #onDiagnostic: ((diagnostic: AndroidDriverDiagnostic) => void) | undefined; readonly #portAllocator: PortAllocator; readonly #processRunner: ProcessRunner; - readonly #profiles = new Map(); + readonly #resolvedProfiles = new Map(); readonly #readinessTimeoutMs: number; readonly #sdk: AndroidSdkPaths; readonly #avdDirectory: string; private constructor(options: AndroidDriverOptions, sdk: AndroidSdkPaths) { + this.#acceptAndroidLicenses = options.acceptAndroidLicenses ?? false; this.#clock = options.clock; + this.#downloadTimeoutMs = options.downloadTimeoutMs ?? DEFAULT_DOWNLOAD_TIMEOUT_MS; this.#filesystem = options.filesystem; this.#hostAbi = options.hostAbi ?? hostAbiFor(process.arch); this.#idGenerator = options.idGenerator ?? new SequentialIdGenerator(); @@ -139,6 +179,9 @@ export class AndroidDriver implements Driver { this.#sdk = sdk; this.#avdDirectory = options.env.ANDROID_AVD_HOME ?? `${options.homeDirectory}/.android/avd`; this.#portAllocator = portAllocatorFor(options.processRunner, sdk.adb); + this.#deviceProfiles = new DeviceProfileRegistry( + options.deviceProfileSources ?? defaultDeviceProfileSources(options, sdk, this.#onDiagnostic), + ); } static async create(options: AndroidDriverOptions): Promise { @@ -158,7 +201,7 @@ export class AndroidDriver implements Driver { throw new Error(`Android driver cannot resolve ${request.platform} requests`); } - const profile = await this.#resolveProfile(request.model); + const profile = await this.#deviceProfiles.resolve(request.model); const images = await this.#installedImages(); const apiLevel = request.osVersion ?? newestApiLevel(images); if (apiLevel === undefined) { @@ -171,12 +214,10 @@ export class AndroidDriver implements Driver { } const packageName = systemImagePackage(apiLevel, "google_apis", this.#hostAbi); - await this.#runOrThrow(this.#sdk.sdkmanager, ["--install", packageName], { - timeoutMs: SDK_DOWNLOAD_TIMEOUT_MS, - }); + await this.#installSystemImage(packageName); } - this.#profiles.set(profile.name.toLocaleLowerCase(), profile); + this.#resolvedProfiles.set(profile.name.toLocaleLowerCase(), profile); return { model: profile.name, osVersion: apiLevel, platform: this.platform }; } @@ -187,6 +228,13 @@ export class AndroidDriver implements Driver { const avdName = `simlock_${this.#idGenerator.generate()}`; const packageName = systemImagePackage(image.apiLevel, image.tag, image.abi); + // A `builtin` profile already carries the `avdmanager` device id `-d` wants. A + // `properties` profile has none -- it never came from `avdmanager list device` -- so + // `avdmanager create avd` is seeded with *some* built-in device (only to skip the + // interactive "custom hardware profile?" prompt) and the profile's own properties then + // overwrite that seed's config.ini values below, before anything reads them. + const seedDeviceId = + profile.kind === "builtin" ? profile.avdmanagerId : await this.#defaultAvdmanagerDeviceId(); await this.#runOrThrow(this.#sdk.avdmanager, [ "create", "avd", @@ -195,9 +243,16 @@ export class AndroidDriver implements Driver { "-k", packageName, "-d", - profile.id, + seedDeviceId, ]); + if (profile.kind === "properties") { + // Must happen before `#configHash` below captures the driver's snapshot/config-hash + // baseline: applying it after would let the baseline settle on the seed device's + // hardware and then see a spurious drift on the very next boot. + await this.#applyHardwareProperties(avdName, profile.hardwareProperties); + } + const configHash = await this.#configHash(avdName, image); const port = await this.#portAllocator.allocate(); const driverData: AndroidDriverData = { @@ -480,13 +535,13 @@ export class AndroidDriver implements Driver { } async listCatalog(): Promise { - const [profiles, images] = await Promise.all([ - this.#listDeviceProfiles(), + const [models, images] = await Promise.all([ + this.#deviceProfiles.listModels(), this.#installedImages(), ]); return { defaultRuntime: newestApiLevel(images), - models: profiles.map((profile) => profile.name), + models: [...models], runtimes: [...new Set(images.map((image) => image.apiLevel))].sort(compareApiLevels), }; } @@ -508,27 +563,98 @@ export class AndroidDriver implements Driver { } } - async #profileFor(model: string): Promise { - return this.#profiles.get(model.toLocaleLowerCase()) ?? this.#resolveProfile(model); + async #profileFor(model: string): Promise { + return ( + this.#resolvedProfiles.get(model.toLocaleLowerCase()) ?? this.#deviceProfiles.resolve(model) + ); } - async #listDeviceProfiles(): Promise { + /** See the seed-device comment at its `provision` call site. */ + async #defaultAvdmanagerDeviceId(): Promise { const result = await this.#runOrThrow(this.#sdk.avdmanager, ["list", "device"]); - return parseDeviceProfiles(result.stdout); + const [first] = parseAvdmanagerDeviceProfiles(result.stdout); + if (first === undefined) { + throw new DriverCrashError( + `${this.#sdk.avdmanager} list device reported no built-in device profiles`, + ); + } + return first.id; } - async #resolveProfile(model: string): Promise { - const profiles = await this.#listDeviceProfiles(); - const normalized = model.toLocaleLowerCase(); - const profile = profiles.find( - (candidate) => - candidate.name.toLocaleLowerCase() === normalized || - candidate.id.toLocaleLowerCase() === normalized, - ); - if (profile === undefined) { - throw new UnknownModelError(this.platform, model); + /** + * Merges `properties` into the AVD's `config.ini`, overwriting any key it already has and + * appending the rest -- same read-modify-write shape as `#writeDurableMark` below. + */ + async #applyHardwareProperties( + avdName: string, + properties: Readonly>, + ): Promise { + const path = this.#configIniPath(avdName); + let contents: string; + try { + contents = await this.#filesystem.readFile(path); + } catch { + contents = ""; + } + const lines = contents === "" ? [] : contents.replace(/\r?\n$/, "").split(/\r?\n/); + for (const [key, value] of Object.entries(properties)) { + const line = `${key}=${value}`; + const existingIndex = lines.findIndex((entry) => entry.startsWith(`${key}=`)); + if (existingIndex >= 0) { + lines[existingIndex] = line; + } else { + lines.push(line); + } + } + await this.#filesystem.writeFileAtomic(path, `${lines.join("\n")}\n`); + } + + /** + * Installs a system image, accepting Android SDK licenses first when `sdkmanager` refuses + * on an unaccepted one and `acceptAndroidLicenses` allows it -- never otherwise: license + * consent is independent of, and never implied by, download permission. + */ + async #installSystemImage(packageName: string): Promise { + const result = await this.#processRunner.run(this.#sdk.sdkmanager, ["--install", packageName], { + timeoutMs: this.#downloadTimeoutMs, + }); + if (result.code === 0 && !hasUnacceptedLicense(result)) { + return; + } + if (!hasUnacceptedLicense(result)) { + throw new DriverCrashError( + `${this.#sdk.sdkmanager} --install ${packageName} failed: ${result.stderr || result.stdout}`, + ); + } + if (!this.#acceptAndroidLicenses) { + throw new AndroidLicenseNotAcceptedError(packageName); + } + + await this.#acceptLicenses(); + + const retry = await this.#processRunner.run(this.#sdk.sdkmanager, ["--install", packageName], { + timeoutMs: this.#downloadTimeoutMs, + }); + if (retry.code !== 0 || hasUnacceptedLicense(retry)) { + throw new DriverCrashError( + `${this.#sdk.sdkmanager} --install ${packageName} still failed after accepting licenses: ` + + `${retry.stderr || retry.stdout}`, + ); + } + } + + async #acceptLicenses(): Promise { + const result = await this.#processRunner.run(this.#sdk.sdkmanager, ["--licenses"], { + // `sdkmanager --licenses` prompts once per outstanding license; answering more times + // than there are real licenses is harmless (see `LICENSE_ACCEPT_ANSWERS`). + input: "y\n".repeat(LICENSE_ACCEPT_ANSWERS), + timeoutMs: this.#downloadTimeoutMs, + }); + if (result.code !== 0) { + throw new DriverCrashError( + `${this.#sdk.sdkmanager} --licenses failed: ${result.stderr || result.stdout}`, + ); } - return profile; } async #installedImages(): Promise { @@ -1038,24 +1164,6 @@ function compareCommandLineToolVersions(left: string, right: string): number { return left.localeCompare(right); } -function parseDeviceProfiles(output: string): DeviceProfile[] { - const profiles: DeviceProfile[] = []; - let id: string | undefined; - for (const line of output.split(/\r?\n/)) { - const idMatch = /^id:\s*\d+\s+or\s+"([^"]+)"/.exec(line.trim()); - if (idMatch?.[1] !== undefined) { - id = idMatch[1]; - continue; - } - const nameMatch = /^Name:\s*(.+)$/.exec(line.trim()); - if (id !== undefined && nameMatch?.[1] !== undefined) { - profiles.push({ id, name: nameMatch[1] }); - id = undefined; - } - } - return profiles; -} - function newestApiLevel(images: readonly SystemImage[]): string | undefined { return [...new Set(images.map((image) => image.apiLevel))].sort(compareApiLevels).at(-1); } @@ -1167,6 +1275,36 @@ function hostAbiFor(architecture: string): string { return architecture === "arm64" ? "arm64-v8a" : "x86_64"; } +/** + * `sdkmanager --install` reports an unaccepted license in its output rather than through a + * dedicated exit code, so this is a best-effort text match against sdkmanager's own wording + * (e.g. `Warning: License for package ... not accepted.` / + * `... licenses have not been accepted.`), checked across both streams since sdkmanager splits + * its output between them across versions. + */ +function hasUnacceptedLicense(result: ProcessResult): boolean { + const combined = `${result.stdout}\n${result.stderr}`; + return /licen[cs]e/i.test(combined) && /not accepted/i.test(combined); +} + +/** + * `[builtin, user]`: `avdmanager list device` first, then a read-only parse of Android + * Studio's `~/.android/devices.xml`. `ANDROID_SDK_HOME` (not `ANDROID_AVD_HOME`, which only + * relocates created AVDs) is the historical env var Android tooling uses to relocate the whole + * `~/.android` directory, including `devices.xml`. + */ +function defaultDeviceProfileSources( + options: AndroidDriverOptions, + sdk: AndroidSdkPaths, + onDiagnostic: ((diagnostic: AndroidDriverDiagnostic) => void) | undefined, +): readonly DeviceProfileSource[] { + const devicesXmlPath = `${options.env.ANDROID_SDK_HOME ?? options.homeDirectory}/.android/devices.xml`; + return [ + new BuiltinDeviceProfileSource(sdk.avdmanager, options.processRunner), + new UserDeviceProfileSource(devicesXmlPath, options.filesystem, onDiagnostic), + ]; +} + function portAllocatorFor(processRunner: ProcessRunner, adb: string): PortAllocator { const existing = allocationsByRunner.get(processRunner); if (existing !== undefined) { diff --git a/src/ports/process-runner.ts b/src/ports/process-runner.ts index 2c4f4bd..a53475a 100644 --- a/src/ports/process-runner.ts +++ b/src/ports/process-runner.ts @@ -24,6 +24,12 @@ export interface ProcessRunOptions { readonly timeoutMs?: number; readonly env?: NodeJS.ProcessEnv; readonly cwd?: string; + /** + * Written to the child's stdin and then closed. For a CLI that reads an interactive prompt + * from stdin (e.g. `sdkmanager --licenses`'s per-license `y/N`) rather than accepting a flag. + * Omitted means stdin is left open and unwritten, exactly as before this option existed. + */ + readonly input?: string; } export interface ProcessResult { @@ -84,6 +90,10 @@ export class NodeProcessRunner implements ProcessRunner { stdio: "pipe", }); + if (options.input !== undefined) { + child.stdin?.end(options.input); + } + return new NodeProcessHandle(child); } } From 47ab2c78be1ff24440ded47d8f01ad20517e6314 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Tue, 1 Sep 2026 16:32:54 +0200 Subject: [PATCH 4/6] feat(events): component install facts, disk preflight, durable install log (#67) Adds component.install-started / component.installed / component.install-failed to the event bus and emits them for both drivers' install paths. Drivers never depend on the bus directly (architecture rule 5): each driver reports the fact through a new/extended onDiagnostic callback (mirroring the Android driver's existing pattern; iOS gains the same option), and src/daemon/main.ts bridges the diagnostic to the bus at driver construction time -- hence the "driver-diagnostics" emitter. install-failed carries a stable error summary matching device.purge-failed's own format; the Android license-retry path emits exactly one install-failed regardless of which branch throws. Adds a disk-space preflight (assertDiskSpace / InsufficientDiskSpaceError in core/driver.ts, shared by both drivers) before either install starts: ~8 GiB for an iOS runtime, ~2 GiB for an Android system image, checked via Filesystem#diskFree. A preflight failure fires no diagnostic and makes no xcodebuild/sdkmanager install call. startDaemon now subscribes logger.child("components") to component.installed so an installed component stays attributable in daemon.log after the event ring buffer resets on restart -- no registry entry or uninstall, per the issue's stated scope. Investigated threading download progress through to the waiting requester's lease-progress stream (item 4): doing so needs a new LeaseProgress stage and a Driver.resolveSpec progress callback both drivers would implement -- real protocol machinery, not a small addition -- so it was documented as a gap in docs/known-pitfalls.md and docs/IDEAS.md instead of built. Updates docs/EVENTS.md (new Components section), docs/ARCHITECTURE.md, docs/CLI.md, and docs/IDEAS.md (adds the deferred Apple-downloadables-index idea stage 2's known-pitfalls entry pointed at, plus the progress-push idea). --- docs/ARCHITECTURE.md | 31 ++++++ docs/CLI.md | 13 ++- docs/EVENTS.md | 18 ++++ docs/IDEAS.md | 28 ++++++ docs/known-pitfalls.md | 22 ++++- src/bus/index.test.ts | 3 + src/bus/index.ts | 12 +++ src/core/driver.ts | 38 ++++++++ src/core/index.ts | 2 + src/daemon/main.test.ts | 129 ++++++++++++++++++++++++- src/daemon/main.ts | 106 +++++++++++++++++++- src/drivers/android/index.test.ts | 154 +++++++++++++++++++++++++++++- src/drivers/android/index.ts | 47 ++++++++- src/drivers/diagnostics.ts | 24 +++++ src/drivers/ios/index.test.ts | 135 ++++++++++++++++++++++++++ src/drivers/ios/index.ts | 67 +++++++++++-- 16 files changed, 816 insertions(+), 13 deletions(-) create mode 100644 src/drivers/diagnostics.ts diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index ba6cd21..3e40abc 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -458,6 +458,29 @@ Events are emitted post-commit only; handler failures are isolated from emitters. See [EVENTS.md](EVENTS.md) and [agent-rules/events.md](agent-rules/events.md). +### Driver facts reach the bus through diagnostics, never directly + +Drivers must never depend on the event bus (architecture rule 5 — a driver is +not an observer of its own facts). Where a driver needs to report something +the daemon should turn into a bus event, it reports it through its own +`onDiagnostic` callback option instead — the Android driver already did this +for `snapshot-cold-boot` and unreadable device-profile sources; the iOS +driver gained the same option for component installs. `src/daemon/main.ts` +wires each driver's `onDiagnostic` at construction time +(`discoverDrivers`), bridging the diagnostic to `component.install-started` / +`component.installed` / `component.install-failed` — see +[EVENTS.md](EVENTS.md#components). This is also why those events are +attributed to the `driver-diagnostics` emitter rather than to `IosSimctlDriver` +or `AndroidDriver` directly: the driver only observed the fact, the daemon +layer is what committed it to the bus. + +Before starting either install, the driver checks free disk space against a +conservative per-component estimate (~8 GiB for an iOS runtime, ~2 GiB for an +Android system image) via `Filesystem#diskFree` and fails fast with a typed +`InsufficientDiskSpaceError` naming required vs. available bytes — no +`component.install-*` diagnostic fires for a preflight failure, since no +install was actually attempted. + ## External APIs behind interfaces (ports) Every external API the app touches gets its own type/interface (a *port*), @@ -561,6 +584,14 @@ cannot depend on `config.log` having loaded successfully, so it builds its own logger straight from the default log path at a fixed level, falling back to `console.error` only if that itself fails. +`startDaemon` also subscribes `logger.child("components")` to `component.installed` +(`wireComponentInstallLogging` in `src/daemon/main.ts`) so a component simlock +installed on an agent's behalf stays attributable in `daemon.log` after the +event ring buffer resets on restart — the same durable-vs-ring-buffer split as +everything else in this section, applied to component installs specifically +because there is no registry entry or uninstall for them to be recovered from +otherwise (see "Out of scope" in the #67 issue). + ## Device requests Required to identify a device: **platform + device model + OS version**. diff --git a/docs/CLI.md b/docs/CLI.md index 8313fcc..bc3f4fc 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -95,7 +95,18 @@ simlock lease --platform --device [--os ] installing/upgrading Xcode by hand. A requested `--os` outside the device's supported runtime range (e.g. iPhone Xs above iOS 18.x) fails immediately — no download is ever attempted for a version that could not - work regardless. + work regardless. For Android, this runs `sdkmanager --install`; an + unaccepted SDK license fails naming `downloads.acceptAndroidLicenses` + (config) unless that flag is set, in which case licenses are accepted + automatically and the install retried once. Both drivers check free disk + space before starting either install and fail fast, naming required vs. + available bytes, instead of risking a full disk mid-download. Every + install attempt (including a license-triggered retry) emits + `component.install-started` / `component.installed` / + `component.install-failed` on the event bus (`simlock events --follow`); + see [EVENTS.md](EVENTS.md#components). The requester's own progress stream + (below) does not yet reflect an in-flight download — see + [known-pitfalls.md](known-pitfalls.md). - `--detach` — detached mode: print the lease result and exit; the lease is TTL-bound and must be renewed with `simlock lease renew`. - `--bind-pid ` — held mode only: watch this pid for death instead of diff --git a/docs/EVENTS.md b/docs/EVENTS.md index 643d89d..7b858c3 100644 --- a/docs/EVENTS.md +++ b/docs/EVENTS.md @@ -40,6 +40,24 @@ in short: `subject.past-tense-fact`, emitted post-commit, facts not commands. | `device.recovered` | device id, lease id, attempts, duration | a crashed leased device was rebooted under its existing lease and passed readiness | LeaseHealthMonitor | implemented | | `device.recovery-failed` | device id, lease id, attempts, reason, error | recovery could not restore a leased device (absent from driver reality, provenance drift, or attempts exhausted) and its lease was released | LeaseHealthMonitor | implemented | +## Components + +| Event | Payload (key fields) | Emitted when | Emitter | Status | +|---|---|---|---|---| +| `component.install-started` | platform, component id (iOS runtime version or "latest"; Android `sdkmanager` package name) | a driver is about to run `xcodebuild -downloadPlatform` / `sdkmanager --install` for a missing component, disk preflight already passed | driver-diagnostics | implemented | +| `component.installed` | platform, component id, duration | the install succeeded | driver-diagnostics | implemented | +| `component.install-failed` | platform, component id, duration, stable error summary | the install failed, including a license-retry failure (exactly one `install-failed` per attempted install, never one per retry) | driver-diagnostics | implemented | + +Drivers never touch the event bus directly (architecture rule 5): both drivers report these +facts through their own `onDiagnostic` callback (mirroring the Android driver's pre-existing +diagnostic pattern), and `src/daemon/main.ts` bridges that diagnostic to the bus at driver +construction time — hence the `driver-diagnostics` emitter rather than `IosSimctlDriver` / +`AndroidDriver`. A disk-preflight failure (`InsufficientDiskSpaceError`) happens before any +diagnostic fires: no install was attempted, so nothing is reported as started or failed. See +"Device requests" and "Fresh-state strategy" in [ARCHITECTURE.md](ARCHITECTURE.md) for how a +missing component gets to this point, and `docs/known-pitfalls.md` for the requester-visible +progress gap this leaves. + ## System | Event | Payload (key fields) | Emitted when | Emitter | Status | diff --git a/docs/IDEAS.md b/docs/IDEAS.md index 951d141..d9d2e60 100644 --- a/docs/IDEAS.md +++ b/docs/IDEAS.md @@ -53,6 +53,34 @@ boundary held. (pre-installed certs, test apps). Offer per-pool config: reclaim by erase (default) or by re-clone from a maintained golden device. +## Parse Apple's downloadables index for exact runtime versions (iOS) + +The bounded-default download path (`IosSimctlDriver#resolveDefaultRuntime`, +`src/drivers/ios/index.ts`) can only ask `xcodebuild -downloadPlatform iOS +-buildVersion ` when no `--os` was given and the model's pairing range +has an upper bound — the bare major version, not an exact patch release, +because the exact downloadable versions for the installed Xcode aren't known +offline. Xcode's own downloadable-runtimes catalog (fetched by Xcode/App +Store internally) would let the driver resolve the exact newest compatible +patch release instead of gambling on the major matching a real build. Not +pursued in v1: parsing an undocumented, Apple-controlled catalog format is a +maintenance burden disproportionate to the edge case it closes (see +`docs/known-pitfalls.md`, "Component downloads"). + +## Requester-visible download progress + +Neither driver's install (`xcodebuild -downloadPlatform`, `sdkmanager +--install`) currently reaches the requesting connection's progress stream — +see `docs/known-pitfalls.md` ("no progress push"). Closing this needs a +`downloading` stage on `LeaseProgress` (`src/core/wait-queue.ts`) and a +`Driver.resolveSpec` progress callback both drivers implement, threaded +through `LeaseAcquisitionCoordinator#resolveAndDrive` the same way +`provision`/`makeReady` already report `provisioning`/`booting`. Deferred +because it is protocol machinery (interface + CLI/MCP wire changes), not a +small addition, and the daemon-side `component.install-started` bus event +already gives an operator visibility via `simlock events --follow` even +though the waiting requester itself does not see it yet. + ## Cross-machine coordination A fleet-level broker over multiple hosts. Explicitly out of scope for v1 diff --git a/docs/known-pitfalls.md b/docs/known-pitfalls.md index 1e0c6fe..d3b2f45 100644 --- a/docs/known-pitfalls.md +++ b/docs/known-pitfalls.md @@ -100,7 +100,7 @@ destroys it (registry-only, as always). The device stays visible as `device.quarantine-recovered`, and `device.quarantine-abandoned` are the new follow-up facts (see `docs/EVENTS.md`). -## iOS runtime downloads: per-request blocking and the bounded-default edge case +## Component downloads: per-request blocking, the bounded-default edge case, and no progress push The iOS driver's `resolveSpec` (`src/drivers/ios/index.ts`) can now run `xcodebuild -downloadPlatform iOS` when a requested runtime is missing and @@ -126,3 +126,23 @@ downloadables index isn't parsed in v1; see `docs/IDEAS.md`). If Xcode doesn't have a build matching that bare major version, the download fails and the caller is told to pass `--os ` explicitly rather than retrying blind. + +**No requester-visible progress during a download (#67 stage 4).** The +requester's lease-progress stream (`LeaseProgress` in `src/core/wait-queue.ts` +— `queued` / `provisioning` / `booting` / `reclaiming`, relayed as CLI stderr +JSON lines and MCP `notifications/progress`) has no `downloading` stage. Both +drivers' `resolveSpec` — where a runtime or system-image install actually +happens — runs before `LeaseAcquisitionCoordinator#drive`'s provisioning +step, and `Driver.resolveSpec`'s signature carries no progress callback the +way `provision`/`makeReady` do. A held-mode CLI or MCP caller waiting on a +multi-minute install today sees nothing on the wire between its request and +either the eventual grant or a timeout; the only visibility is the daemon's +own `component.install-started` bus event (`simlock events --follow`) and log +line, neither reaching the waiting connection itself. Threading a +`downloading` stage through would mean widening the `Driver` interface +(`resolveSpec` gaining an `onProgress`-shaped option, both drivers +implementing it), a new `LeaseProgress` variant, and CLI/MCP wire changes — +real protocol machinery, not a small addition, so it was deliberately not +built in stage 4. `component.install-started`'s payload already carries +enough (`platform`, `componentId`) that a future pass wiring this through +would mostly be plumbing, not new information to invent. diff --git a/src/bus/index.test.ts b/src/bus/index.test.ts index 95f0b07..009dadd 100644 --- a/src/bus/index.test.ts +++ b/src/bus/index.test.ts @@ -133,6 +133,9 @@ describe("EventBus", () => { | "device.quarantine-stranded" | "device.shutdown" | "device.deleted" + | "component.install-started" + | "component.installed" + | "component.install-failed" | "device.foreign-state-detected" | "device.foreign-provenance-detected" | "device.stalled-transition-detected" diff --git a/src/bus/index.ts b/src/bus/index.ts index 254eb3c..11009f1 100644 --- a/src/bus/index.ts +++ b/src/bus/index.ts @@ -86,6 +86,18 @@ export interface EventMap { }; "device.shutdown": { readonly deviceId: string; readonly initiator: string }; "device.deleted": { readonly deviceId: string; readonly initiator: string }; + "component.install-started": { readonly platform: string; readonly componentId: string }; + "component.installed": { + readonly platform: string; + readonly componentId: string; + readonly durationMs: number; + }; + "component.install-failed": { + readonly platform: string; + readonly componentId: string; + readonly durationMs: number; + readonly error: string; + }; "daemon.started": { readonly version: string; readonly configSnapshot: unknown }; "daemon.stopping": { readonly reason: string }; "disk.pressure-detected": { readonly freeBytes: number; readonly threshold: number }; diff --git a/src/core/driver.ts b/src/core/driver.ts index 022f67f..0afcbe8 100644 --- a/src/core/driver.ts +++ b/src/core/driver.ts @@ -1,3 +1,4 @@ +import type { Filesystem } from "../ports/index.js"; import type { DeviceSpec, Platform } from "./domain.js"; export interface DeviceRequest { @@ -156,3 +157,40 @@ export class DriverCrashError extends Error { this.name = "DriverCrashError"; } } + +export class InsufficientDiskSpaceError extends Error { + constructor( + readonly platform: Platform, + readonly requiredBytes: number, + readonly availableBytes: number, + ) { + super( + `Not enough free disk space to install a ${platform} component: needs ~` + + `${formatGibibytes(requiredBytes)} free, only ${formatGibibytes(availableBytes)} available`, + ); + this.name = "InsufficientDiskSpaceError"; + } +} + +function formatGibibytes(bytes: number): string { + return `${(bytes / 1024 ** 3).toFixed(1)} GiB`; +} + +/** + * Checked before a driver starts any multi-GB component download/install, so a full disk fails + * fast with a clear message instead of filling up mid-download (see safety rule 4's spirit -- + * downloads must never surprise the machine they run on). `path` defaults to `"."`, the same + * convention `CleanupReaper` uses for its own disk-pressure check (`src/core/reaper.ts`): the + * daemon process's own working-directory volume. + */ +export async function assertDiskSpace( + filesystem: Pick, + platform: Platform, + requiredBytes: number, + path = ".", +): Promise { + const availableBytes = await filesystem.diskFree(path); + if (availableBytes < requiredBytes) { + throw new InsufficientDiskSpaceError(platform, requiredBytes, availableBytes); + } +} diff --git a/src/core/index.ts b/src/core/index.ts index 7d1d3e9..667d279 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -18,6 +18,7 @@ export { type CleanupRule, type RegistryView } from "./cleanup/types.js"; export { automaticCleanupRules } from "./cleanup/rules.js"; export { CleanupReaper } from "./reaper.js"; export { + assertDiskSpace, BootTimeoutError, type DeviceRequest, type Driver, @@ -26,6 +27,7 @@ export { type DriverDevice, type DriverEstimate, type DriverReality, + InsufficientDiskSpaceError, type ObservedDevice, type ObservedRunState, RuntimeMissingError, diff --git a/src/daemon/main.test.ts b/src/daemon/main.test.ts index afac58d..3dc14ad 100644 --- a/src/daemon/main.test.ts +++ b/src/daemon/main.test.ts @@ -4,6 +4,7 @@ import { join } from "node:path"; import { connect } from "node:net"; import { afterEach, describe, expect, it } from "vitest"; +import { EventBus } from "../bus/index.js"; import { FakeDriver } from "../core/index.js"; import { DAEMON_PROTOCOL_VERSION } from "../daemon-protocol/index.js"; import { @@ -14,7 +15,14 @@ import { MemoryLogSink, ScriptedProcessRunner, } from "../ports/index.js"; -import { discoverDrivers, startDaemon, type StartDaemonOptions } from "./main.js"; +import { + bridgeAndroidDriverDiagnostic, + discoverDrivers, + emitComponentInstallDiagnostic, + startDaemon, + wireComponentInstallLogging, + type StartDaemonOptions, +} from "./main.js"; import type { DaemonServer } from "./server.js"; const runningDaemons: DaemonServer[] = []; @@ -149,6 +157,7 @@ describe("discoverDrivers", () => { const drivers = await discoverDrivers({ clock: new FakeClock(), + eventBus: new EventBus(new FakeClock()), filesystem, idGenerator: new CryptoIdGenerator(), logger, @@ -166,6 +175,123 @@ describe("discoverDrivers", () => { }); }); +describe("component install diagnostic bridging", () => { + it("emits component.install-started/-installed/-failed for the bridged platform", () => { + const clock = new FakeClock(1_000); + const eventBus = new EventBus(clock); + const seen: unknown[] = []; + eventBus.subscribeAll((envelope) => seen.push(envelope)); + const bridge = emitComponentInstallDiagnostic(eventBus, "ios"); + + bridge({ componentId: "18.6", kind: "component-install-started" }); + bridge({ componentId: "18.6", durationMs: 42_000, kind: "component-installed" }); + bridge({ + componentId: "18.6", + durationMs: 5_000, + error: "DriverCrashError: xcodebuild failed", + kind: "component-install-failed", + }); + + expect(seen).toEqual([ + expect.objectContaining({ + event: "component.install-started", + module: "driver-diagnostics", + payload: { componentId: "18.6", platform: "ios" }, + }), + expect.objectContaining({ + event: "component.installed", + module: "driver-diagnostics", + payload: { componentId: "18.6", durationMs: 42_000, platform: "ios" }, + }), + expect.objectContaining({ + event: "component.install-failed", + module: "driver-diagnostics", + payload: { + componentId: "18.6", + durationMs: 5_000, + error: "DriverCrashError: xcodebuild failed", + platform: "ios", + }, + }), + ]); + }); + + it("forwards only component-install-* diagnostics from the Android driver's broader onDiagnostic surface", () => { + const clock = new FakeClock(1_000); + const eventBus = new EventBus(clock); + const seen: unknown[] = []; + eventBus.subscribeAll((envelope) => seen.push(envelope)); + const bridge = bridgeAndroidDriverDiagnostic(eventBus); + + bridge({ avdName: "simlock_1", kind: "snapshot-cold-boot", readyAfterMs: 15_000 }); + bridge({ + kind: "device-profile-source-unreadable", + path: "/x/.android/devices.xml", + reason: "parse-error", + }); + bridge({ + componentId: "system-images;android-35;google_apis;arm64-v8a", + kind: "component-install-started", + }); + + expect(seen).toEqual([ + expect.objectContaining({ + event: "component.install-started", + payload: { + componentId: "system-images;android-35;google_apis;arm64-v8a", + platform: "android", + }, + }), + ]); + }); +}); + +describe("wireComponentInstallLogging", () => { + it('writes a durable structured log line under logger.child("components") when component.installed fires', () => { + const clock = new FakeClock(1_000); + const sink = new MemoryLogSink(); + const logger = new JsonLinesLogger({ clock, level: "debug", sink }); + const eventBus = new EventBus(clock); + + wireComponentInstallLogging(eventBus, logger); + eventBus.emit( + "component.installed", + { componentId: "18.6", durationMs: 42_000, platform: "ios" }, + "driver-diagnostics", + ); + + expect(sink.records).toContainEqual( + expect.objectContaining({ + level: "info", + message: "Component installed", + module: "daemon.components", + fields: { componentId: "18.6", durationMs: 42_000, platform: "ios" }, + }), + ); + }); + + it("does not log for component.install-started or component.install-failed", () => { + const clock = new FakeClock(1_000); + const sink = new MemoryLogSink(); + const logger = new JsonLinesLogger({ clock, level: "debug", sink }); + const eventBus = new EventBus(clock); + + wireComponentInstallLogging(eventBus, logger); + eventBus.emit( + "component.install-started", + { componentId: "18.6", platform: "ios" }, + "driver-diagnostics", + ); + eventBus.emit( + "component.install-failed", + { componentId: "18.6", durationMs: 1_000, error: "boom", platform: "ios" }, + "driver-diagnostics", + ); + + expect(sink.records).toEqual([]); + }); +}); + describe("discoverDrivers with SIMLOCK_DRIVERS_MODULE", () => { const previousModule = process.env.SIMLOCK_DRIVERS_MODULE; @@ -189,6 +315,7 @@ describe("discoverDrivers with SIMLOCK_DRIVERS_MODULE", () => { const logger = new JsonLinesLogger({ clock: new FakeClock(), level: "debug", sink }); return discoverDrivers({ clock: new FakeClock(), + eventBus: new EventBus(new FakeClock()), filesystem: new MemoryFilesystem(), idGenerator: new CryptoIdGenerator(), logger, diff --git a/src/daemon/main.ts b/src/daemon/main.ts index b718f40..854fee8 100644 --- a/src/daemon/main.ts +++ b/src/daemon/main.ts @@ -13,7 +13,12 @@ import { Registry, Nuke, } from "../core/index.js"; -import { AndroidDriver, SdkMissingError } from "../drivers/android/index.js"; +import { + AndroidDriver, + SdkMissingError, + type AndroidDriverDiagnostic, +} from "../drivers/android/index.js"; +import type { ComponentInstallDiagnostic } from "../drivers/diagnostics.js"; import { IosSimctlDriver } from "../drivers/ios/index.js"; import { CryptoIdGenerator, @@ -85,6 +90,10 @@ export async function startDaemon(options: StartDaemonOptions = {}): Promise; readonly filesystem: Filesystem; readonly idGenerator: IdGenerator; readonly logger: Logger; @@ -208,6 +220,7 @@ export async function discoverDrivers(options: DriverDiscoveryContext): Promise< : { downloadTimeoutMs: options.downloadTimeoutMs }), filesystem: options.filesystem, idGenerator: options.idGenerator, + onDiagnostic: emitComponentInstallDiagnostic(options.eventBus, "ios"), processRunner: options.processRunner, }), ); @@ -225,6 +238,7 @@ export async function discoverDrivers(options: DriverDiscoveryContext): Promise< filesystem: options.filesystem, homeDirectory: homedir(), idGenerator: options.idGenerator, + onDiagnostic: bridgeAndroidDriverDiagnostic(options.eventBus), processRunner: options.processRunner, }), ); @@ -274,6 +288,96 @@ async function loadDriversModule( return drivers; } +/** + * Turns a driver's `component-install-*` diagnostic into the matching `component.install-*` + * bus event. Drivers never depend on the event bus directly (architecture rule 5 -- loose + * coupling via the bus is for observers only) -- this is the one place, at driver construction, + * that bridges the driver's diagnostic callback to a post-commit fact for observers (`simlock + * events`, and the durable-log subscription in `startDaemon`). + */ +export function emitComponentInstallDiagnostic( + eventBus: Pick, + platform: "android" | "ios", +): (diagnostic: ComponentInstallDiagnostic) => void { + return (diagnostic) => { + switch (diagnostic.kind) { + case "component-install-started": + eventBus.emit( + "component.install-started", + { componentId: diagnostic.componentId, platform }, + "driver-diagnostics", + ); + return; + case "component-installed": + eventBus.emit( + "component.installed", + { componentId: diagnostic.componentId, durationMs: diagnostic.durationMs, platform }, + "driver-diagnostics", + ); + return; + case "component-install-failed": + eventBus.emit( + "component.install-failed", + { + componentId: diagnostic.componentId, + durationMs: diagnostic.durationMs, + error: diagnostic.error, + platform, + }, + "driver-diagnostics", + ); + return; + } + }; +} + +function isComponentInstallDiagnostic(diagnostic: { + readonly kind: string; +}): diagnostic is ComponentInstallDiagnostic { + return ( + diagnostic.kind === "component-install-started" || + diagnostic.kind === "component-installed" || + diagnostic.kind === "component-install-failed" + ); +} + +/** + * The Android driver's `onDiagnostic` also carries `snapshot-cold-boot` and + * `device-profile-source-unreadable` facts, neither wired to the bus (unchanged from before + * this change -- discovery never passed `onDiagnostic` to the Android driver at all, so every + * diagnostic it ever reported was already dropped). Only `component-install-*` is bridged here. + */ +export function bridgeAndroidDriverDiagnostic( + eventBus: Pick, +): (diagnostic: AndroidDriverDiagnostic) => void { + const installBridge = emitComponentInstallDiagnostic(eventBus, "android"); + return (diagnostic) => { + if (isComponentInstallDiagnostic(diagnostic)) { + installBridge(diagnostic); + } + }; +} + +/** + * Durable bookkeeping for component installs: the event ring buffer (`simlock events`) resets + * on daemon restart, so a component simlock installed on an agent's behalf is only attributable + * later through this log line -- see the `Logger` port ("Operational logging is a separate + * concern from the event bus" in ARCHITECTURE.md). + */ +export function wireComponentInstallLogging( + eventBus: Pick, + logger: Logger, +): void { + const componentsLogger = logger.child("components"); + eventBus.subscribe("component.installed", (envelope) => { + componentsLogger.info("Component installed", { + componentId: envelope.payload.componentId, + durationMs: envelope.payload.durationMs, + platform: envelope.payload.platform, + }); + }); +} + /** * Best-effort logger for the fatal startup handler below. It cannot depend on the * daemon's own `Config` — that is exactly what may have failed to load — so it always diff --git a/src/drivers/android/index.test.ts b/src/drivers/android/index.test.ts index d59fb73..a8d1e76 100644 --- a/src/drivers/android/index.test.ts +++ b/src/drivers/android/index.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi } from "vitest"; import type { Driver } from "../../core/driver.js"; +import { InsufficientDiskSpaceError } from "../../core/index.js"; import { FakeClock, type Filesystem, @@ -1076,6 +1077,156 @@ describe("AndroidDriver", () => { }); }); + describe("component install diagnostics", () => { + it("reports component-install-started then component-installed with a duration on a clean install", async () => { + const filesystem = await androidFilesystem(); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.sdkmanager, [ + "--install", + "system-images;android-35;google_apis;arm64-v8a", + ]), + ]); + const diagnostics: AndroidDriverDiagnostic[] = []; + const driver = await createDriver(filesystem, runner, { + onDiagnostic: (diagnostic) => diagnostics.push(diagnostic), + }); + + await driver.resolveSpec( + { model: "Pixel 8", osVersion: "35", platform: "android" }, + { allowDownload: true }, + ); + + expect(diagnostics).toEqual([ + { + componentId: "system-images;android-35;google_apis;arm64-v8a", + kind: "component-install-started", + }, + { + componentId: "system-images;android-35;google_apis;arm64-v8a", + durationMs: 0, + kind: "component-installed", + }, + ]); + }); + + it("reports component-install-failed with a stable error summary when the install is rejected outright", async () => { + const filesystem = await androidFilesystem(); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + { + match: { + args: ["--install", "system-images;android-35;google_apis;arm64-v8a"], + command: binaries.sdkmanager, + }, + result: { code: 1, stderr: "no network", stdout: "" }, + }, + ]); + const diagnostics: AndroidDriverDiagnostic[] = []; + const driver = await createDriver(filesystem, runner, { + onDiagnostic: (diagnostic) => diagnostics.push(diagnostic), + }); + + await driver + .resolveSpec( + { model: "Pixel 8", osVersion: "35", platform: "android" }, + { allowDownload: true }, + ) + .catch((error: unknown) => error); + + expect(diagnostics).toEqual([ + { + componentId: "system-images;android-35;google_apis;arm64-v8a", + kind: "component-install-started", + }, + { + componentId: "system-images;android-35;google_apis;arm64-v8a", + durationMs: 0, + error: expect.stringContaining("DriverCrashError:"), + kind: "component-install-failed", + }, + ]); + }); + + it("reports exactly one component-install-failed for a license-retry failure, not one per attempt", async () => { + const licenseNotAcceptedOutput = + "Warning: License for package Android SDK Platform 35 not accepted.\n\n" + + "1 package(s) were skipped due to license issues. Please accept the license(s) and try " + + "again.\nTo resolve, run: sdkmanager --licenses\n"; + const filesystem = await androidFilesystem(); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + { + match: { + args: ["--install", "system-images;android-35;google_apis;arm64-v8a"], + command: binaries.sdkmanager, + }, + result: { code: 1, stderr: "", stdout: licenseNotAcceptedOutput }, + }, + processResult(binaries.sdkmanager, ["--licenses"], "All licenses accepted.\n"), + { + match: { + args: ["--install", "system-images;android-35;google_apis;arm64-v8a"], + command: binaries.sdkmanager, + }, + result: { code: 1, stderr: "still refusing", stdout: "" }, + }, + ]); + const diagnostics: AndroidDriverDiagnostic[] = []; + const driver = await createDriver(filesystem, runner, { + acceptAndroidLicenses: true, + onDiagnostic: (diagnostic) => diagnostics.push(diagnostic), + }); + + await driver + .resolveSpec( + { model: "Pixel 8", osVersion: "35", platform: "android" }, + { allowDownload: true }, + ) + .catch((error: unknown) => error); + + const installDiagnostics = diagnostics.filter((diagnostic) => + diagnostic.kind.startsWith("component-install"), + ); + expect(installDiagnostics).toEqual([ + { + componentId: "system-images;android-35;google_apis;arm64-v8a", + kind: "component-install-started", + }, + { + componentId: "system-images;android-35;google_apis;arm64-v8a", + durationMs: 0, + error: expect.stringContaining("DriverCrashError:"), + kind: "component-install-failed", + }, + ]); + }); + + it("fails disk preflight before ever invoking sdkmanager, and reports no diagnostic", async () => { + const filesystem = await androidFilesystem({ freeDiskBytes: 1024 }); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + ]); + const diagnostics: AndroidDriverDiagnostic[] = []; + const driver = await createDriver(filesystem, runner, { + onDiagnostic: (diagnostic) => diagnostics.push(diagnostic), + }); + + const error = await driver + .resolveSpec( + { model: "Pixel 8", osVersion: "35", platform: "android" }, + { allowDownload: true }, + ) + .catch((caught: unknown) => caught); + + expect(error).toBeInstanceOf(InsufficientDiskSpaceError); + expect((error as Error).message).toMatch(/needs ~2\.0 GiB.*only 0\.0 GiB available/); + // The device-profile lookup ran (avdmanager list device), but no sdkmanager call at all. + expect(runner.calls.some((call) => call.command === binaries.sdkmanager)).toBe(false); + expect(diagnostics).toEqual([]); + }); + }); + describe("download timeout", () => { it("threads the configured downloadTimeoutMs into the sdkmanager install call", async () => { const filesystem = await androidFilesystem(); @@ -1316,10 +1467,11 @@ function customDeviceXml(name: string, ramMiB: number): string { async function androidFilesystem( options: { readonly config?: string; + readonly freeDiskBytes?: number; readonly images?: readonly (readonly [string, string, string])[]; } = {}, ): Promise { - const filesystem = new MemoryFilesystem(); + const filesystem = new MemoryFilesystem(options.freeDiskBytes); for (const binary of Object.values(binaries)) { await filesystem.mkdirp(binary.slice(0, binary.lastIndexOf("/"))); await filesystem.writeFileAtomic(binary, "binary"); diff --git a/src/drivers/android/index.ts b/src/drivers/android/index.ts index 97a2932..5db2a2c 100644 --- a/src/drivers/android/index.ts +++ b/src/drivers/android/index.ts @@ -1,5 +1,6 @@ import type { DeviceSpec } from "../../core/domain.js"; import { + assertDiskSpace, BootTimeoutError, type DeviceRequest, type Driver, @@ -13,6 +14,7 @@ import { type ReclaimResult, RuntimeMissingError, } from "../../core/driver.js"; +import type { ComponentInstallDiagnostic } from "../diagnostics.js"; import type { Clock, Filesystem, @@ -52,6 +54,9 @@ const LICENSE_ACCEPT_ANSWERS = 100; // from ever turning a "we already killed it" cleanup into an unbounded await. const SIGKILL_REAP_TIMEOUT_MS = 5_000; const SNAPSHOT_BOOT_ESTIMATE_MS = 4_000; +// Conservative estimate for a system-image download+install -- checked before `sdkmanager +// --install` ever starts, so a full disk fails fast instead of filling up mid-download. +const ANDROID_SYSTEM_IMAGE_MIN_FREE_BYTES = 2 * 1024 ** 3; const PROVISION_ESTIMATE_MS = 1_000; // Measured on an M3 Pro against Pixel 8 / API 35: 2.4-5.1s over nine steady-state reclaims // (median 4.6s), and 3.7-5.7s with three running at once. A `snapshot` reclaim loads the clean @@ -101,7 +106,8 @@ export interface AndroidDriverOptions { export type AndroidDriverDiagnostic = | { readonly avdName: string; readonly kind: "snapshot-cold-boot"; readonly readyAfterMs: number } - | DeviceProfileSourceDiagnostic; + | DeviceProfileSourceDiagnostic + | ComponentInstallDiagnostic; export class SdkMissingError extends Error { constructor(readonly searchedPaths: readonly string[]) { @@ -609,12 +615,43 @@ export class AndroidDriver implements Driver { await this.#filesystem.writeFileAtomic(path, `${lines.join("\n")}\n`); } + /** + * Disk preflight, then the actual `sdkmanager` install, wrapped with `component.install-*` + * diagnostics -- split from `#installSystemImageOrThrow` below so the license-retry branching + * stays its own single-responsibility function rather than growing this one's complexity. A + * preflight failure is reported before any diagnostic fires: no install was actually + * attempted, so there is nothing to report as started or failed. The try/catch means a caller + * sees exactly one `install-failed` regardless of which branch below throws, never one per + * attempt. + */ + async #installSystemImage(packageName: string): Promise { + await assertDiskSpace(this.#filesystem, this.platform, ANDROID_SYSTEM_IMAGE_MIN_FREE_BYTES); + this.#onDiagnostic?.({ componentId: packageName, kind: "component-install-started" }); + const startedAt = this.#clock.now(); + try { + await this.#installSystemImageOrThrow(packageName); + } catch (error: unknown) { + this.#onDiagnostic?.({ + componentId: packageName, + durationMs: this.#clock.now() - startedAt, + error: stableError(error), + kind: "component-install-failed", + }); + throw error; + } + this.#onDiagnostic?.({ + componentId: packageName, + durationMs: this.#clock.now() - startedAt, + kind: "component-installed", + }); + } + /** * Installs a system image, accepting Android SDK licenses first when `sdkmanager` refuses * on an unaccepted one and `acceptAndroidLicenses` allows it -- never otherwise: license * consent is independent of, and never implied by, download permission. */ - async #installSystemImage(packageName: string): Promise { + async #installSystemImageOrThrow(packageName: string): Promise { const result = await this.#processRunner.run(this.#sdk.sdkmanager, ["--install", packageName], { timeoutMs: this.#downloadTimeoutMs, }); @@ -1287,6 +1324,12 @@ function hasUnacceptedLicense(result: ProcessResult): boolean { return /licen[cs]e/i.test(combined) && /not accepted/i.test(combined); } +/** Matches `stableError` in `warm-pool-coordinator.ts` -- `device.purge-failed`'s own summary shape. */ +function stableError(error: unknown): string { + const value = error instanceof Error ? error : new Error(String(error)); + return `${value.name}: ${value.message}`; +} + /** * `[builtin, user]`: `avdmanager list device` first, then a read-only parse of Android * Studio's `~/.android/devices.xml`. `ANDROID_SDK_HOME` (not `ANDROID_AVD_HOME`, which only diff --git a/src/drivers/diagnostics.ts b/src/drivers/diagnostics.ts new file mode 100644 index 0000000..159470b --- /dev/null +++ b/src/drivers/diagnostics.ts @@ -0,0 +1,24 @@ +/** + * Diagnostic shape both drivers report through their own `onDiagnostic` callback when a + * platform component (an iOS runtime, an Android system image) installs. Deliberately neutral + * -- nothing here references simctl or avdmanager concepts (architecture rule 2), so both + * driver modules can depend on it without depending on each other. + * + * Drivers never touch the event bus directly (architecture rule 5: "loose coupling via the bus + * is for observers only" -- a driver is not an observer of its own facts). The daemon layer + * bridges this diagnostic to the `component.install-*` bus events at driver construction time + * -- see `emitComponentInstallDiagnostic` in `src/daemon/main.ts`. + */ +export type ComponentInstallDiagnostic = + | { readonly kind: "component-install-started"; readonly componentId: string } + | { + readonly kind: "component-installed"; + readonly componentId: string; + readonly durationMs: number; + } + | { + readonly kind: "component-install-failed"; + readonly componentId: string; + readonly durationMs: number; + readonly error: string; + }; diff --git a/src/drivers/ios/index.test.ts b/src/drivers/ios/index.test.ts index 8d854a4..51bd896 100644 --- a/src/drivers/ios/index.test.ts +++ b/src/drivers/ios/index.test.ts @@ -5,6 +5,7 @@ import { describe, expect, it } from "vitest"; import { BootTimeoutError, DriverCrashError, + InsufficientDiskSpaceError, RuntimeMissingError, UnknownModelError, } from "../../core/index.js"; @@ -17,6 +18,7 @@ import { SystemClock, type Filesystem, } from "../../ports/index.js"; +import type { ComponentInstallDiagnostic } from "../diagnostics.js"; import { IosSimctlDriver } from "./index.js"; const listFixture = readFileSync(new URL("./fixtures/simctl-list.json", import.meta.url), "utf8"); @@ -257,6 +259,137 @@ describe("IosSimctlDriver", () => { expect(runner.calls.filter((call) => call.command === "xcodebuild")).toHaveLength(1); }); + describe("component install diagnostics", () => { + it("reports component-install-started then component-installed with a duration on a successful download", async () => { + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixture } }, + { + match: { + command: "xcodebuild", + args: ["-downloadPlatform", "iOS", "-buildVersion", "18.6"], + }, + }, + { + match: listInvocation, + result: { code: 0, stderr: "", stdout: listFixtureAfterDownload }, + }, + ]); + const clock = new FakeClock(); + const diagnostics: ComponentInstallDiagnostic[] = []; + const driver = createDriver(runner, clock, new MemoryFilesystem(), (diagnostic) => + diagnostics.push(diagnostic), + ); + + await driver.resolveSpec( + { model: "iPhone 16", osVersion: "18.6", platform: "ios" }, + { allowDownload: true }, + ); + + expect(diagnostics).toEqual([ + { componentId: "18.6", kind: "component-install-started" }, + { componentId: "18.6", durationMs: 0, kind: "component-installed" }, + ]); + }); + + it("reports component-install-failed with a stable error summary when xcodebuild fails", async () => { + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixture } }, + { + match: { + command: "xcodebuild", + args: ["-downloadPlatform", "iOS", "-buildVersion", "18.6"], + }, + result: { code: 1, stderr: "no network", stdout: "" }, + }, + ]); + const diagnostics: ComponentInstallDiagnostic[] = []; + const driver = createDriver(runner, new FakeClock(), new MemoryFilesystem(), (diagnostic) => + diagnostics.push(diagnostic), + ); + + await driver + .resolveSpec( + { model: "iPhone 16", osVersion: "18.6", platform: "ios" }, + { allowDownload: true }, + ) + .catch((error: unknown) => error); + + expect(diagnostics).toEqual([ + { componentId: "18.6", kind: "component-install-started" }, + { + componentId: "18.6", + durationMs: 0, + error: expect.stringContaining("DriverCrashError:"), + kind: "component-install-failed", + }, + ]); + }); + + it('reports "latest" as the component id for an unbounded default-runtime download', async () => { + // A device type with no upper bound (maxRuntimeVersion unbounded) but no currently + // installed runtime lists it as supported -- forces the "no paired runtime, download + // latest" branch rather than the exact-version one the other tests exercise. + const unpairedCatalog = JSON.stringify({ + devicetypes: [ + { + identifier: "com.apple.CoreSimulator.SimDeviceType.iPhone-16", + maxRuntimeVersion: 16_777_215, + minRuntimeVersion: 917_504, + name: "iPhone 16", + }, + ], + runtimes: [ + { + identifier: "com.apple.CoreSimulator.SimRuntime.iOS-18-4", + isAvailable: true, + name: "iOS 18.4", + supportedDeviceTypes: [], + version: "18.4", + }, + ], + }); + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: unpairedCatalog } }, + { match: { command: "xcodebuild", args: ["-downloadPlatform", "iOS"] } }, + { match: listInvocation, result: { code: 0, stderr: "", stdout: unpairedCatalog } }, + ]); + const diagnostics: ComponentInstallDiagnostic[] = []; + const driver = createDriver(runner, new FakeClock(), new MemoryFilesystem(), (diagnostic) => + diagnostics.push(diagnostic), + ); + + await driver + .resolveSpec({ model: "iPhone 16", platform: "ios" }, { allowDownload: true }) + .catch(() => undefined); + + expect(diagnostics[0]).toEqual({ componentId: "latest", kind: "component-install-started" }); + }); + + it("fails disk preflight before ever invoking xcodebuild, and reports no diagnostic", async () => { + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixture } }, + ]); + const diagnostics: ComponentInstallDiagnostic[] = []; + const filesystem = new MemoryFilesystem(1024); + const driver = createDriver(runner, new FakeClock(), filesystem, (diagnostic) => + diagnostics.push(diagnostic), + ); + + const error = await driver + .resolveSpec( + { model: "iPhone 16", osVersion: "18.6", platform: "ios" }, + { allowDownload: true }, + ) + .catch((caught: unknown) => caught); + + expect(error).toBeInstanceOf(InsufficientDiskSpaceError); + expect((error as Error).message).toMatch(/needs ~8\.0 GiB.*only 0\.0 GiB available/); + // Only the initial catalog list happened: no xcodebuild invocation, no diagnostic. + expect(runner.calls.map((call) => call.command)).toEqual(["xcrun"]); + expect(diagnostics).toEqual([]); + }); + }); + it("provisions with the exact simctl argv and returns opaque iOS driver data", async () => { const runner = new ScriptedProcessRunner([ { @@ -712,11 +845,13 @@ function createDriver( runner: ScriptedProcessRunner, clock = new FakeClock(), filesystem: Filesystem = new MemoryFilesystem(), + onDiagnostic?: (diagnostic: ComponentInstallDiagnostic) => void, ): IosSimctlDriver { return new IosSimctlDriver({ clock, filesystem, idGenerator: { generate: () => "device-1" }, + ...(onDiagnostic === undefined ? {} : { onDiagnostic }), processRunner: runner, }); } diff --git a/src/drivers/ios/index.ts b/src/drivers/ios/index.ts index 19962ff..3d0cd68 100644 --- a/src/drivers/ios/index.ts +++ b/src/drivers/ios/index.ts @@ -1,4 +1,5 @@ import { + assertDiskSpace, BootTimeoutError, type DeviceRequest, type Driver, @@ -21,6 +22,7 @@ import type { ProcessResult, ProcessRunner, } from "../../ports/index.js"; +import type { ComponentInstallDiagnostic } from "../diagnostics.js"; const COMMAND_TIMEOUT_MS = 30_000; const BOOTSTATUS_TIMEOUT_MS = 120_000; @@ -35,6 +37,10 @@ const UNBOUNDED_VERSION = 0xff_ff_ff; // `xcodebuild -downloadPlatform iOS -buildVersion` only reaches back to iOS 16.0 (Xcode // 16.1+); older runtimes must be installed through Xcode itself. const IOS_DOWNLOAD_FLOOR: readonly [number, number, number] = [16, 0, 0]; +// Conservative estimate for a simulator runtime download+install (~7 GB observed, rounded up +// with headroom) -- checked before `xcodebuild -downloadPlatform` ever starts, so a full disk +// fails fast instead of filling up mid-download. +const IOS_RUNTIME_MIN_FREE_BYTES = 8 * 1024 ** 3; // A cold `simctl boot` to `bootstatus` measures roughly 30s on a fast, idle machine and up to // a minute on a loaded or slower one. The upper end is the estimate, deliberately: this number // is what a waiting requester is quoted, and quoting 30s to someone who then waits 60s is the @@ -60,6 +66,12 @@ export interface IosSimctlDriverOptions { readonly downloadTimeoutMs?: number; readonly filesystem: Filesystem; readonly idGenerator: IdGenerator; + /** + * Reports `component.install-*` facts for the daemon layer to bridge onto the event bus -- + * this driver never depends on the bus directly (architecture rule 5). Mirrors the Android + * driver's `onDiagnostic` option. + */ + readonly onDiagnostic?: (diagnostic: ComponentInstallDiagnostic) => void; readonly processRunner: ProcessRunner; } @@ -104,6 +116,7 @@ export class IosSimctlDriver implements Driver { readonly #downloadTimeoutMs: number; readonly #filesystem: Filesystem; readonly #idGenerator: IdGenerator; + readonly #onDiagnostic: ((diagnostic: ComponentInstallDiagnostic) => void) | undefined; readonly #processRunner: ProcessRunner; readonly #resolvedSpecs = new Map(); #devicesRoot: string | undefined; @@ -113,6 +126,7 @@ export class IosSimctlDriver implements Driver { this.#downloadTimeoutMs = options.downloadTimeoutMs ?? DEFAULT_DOWNLOAD_TIMEOUT_MS; this.#filesystem = options.filesystem; this.#idGenerator = options.idGenerator; + this.#onDiagnostic = options.onDiagnostic; this.#processRunner = options.processRunner; } @@ -170,7 +184,12 @@ export class IosSimctlDriver implements Driver { ); } - await this.#downloadRuntime(["-downloadPlatform", "iOS", "-buildVersion", osVersion]); + await this.#downloadRuntime(osVersion, [ + "-downloadPlatform", + "iOS", + "-buildVersion", + osVersion, + ]); const refreshed = await this.#loadCatalog(); const runtime = findInstalledRuntime(refreshed, osVersion); if (runtime === undefined) { @@ -208,11 +227,11 @@ export class IosSimctlDriver implements Driver { if (isUnboundedMax(deviceType.maxRuntimeVersion)) { // No upper bound on this model's pairing range: any released version works, so there is // nothing more specific to ask for than "latest". - await this.#downloadRuntime(["-downloadPlatform", "iOS"]); + await this.#downloadRuntime("latest", ["-downloadPlatform", "iOS"]); } else { const major = majorVersionString(deviceType.maxRuntimeVersion); try { - await this.#downloadRuntime(["-downloadPlatform", "iOS", "-buildVersion", major]); + await this.#downloadRuntime(major, ["-downloadPlatform", "iOS", "-buildVersion", major]); } catch (error: unknown) { throw new DriverCrashError( `Could not download a default iOS runtime for ${deviceType.name} (tried ${major}): ` + @@ -247,16 +266,19 @@ export class IosSimctlDriver implements Driver { * callers that ask for the exact same invocation behind one in-flight promise -- mirrors the * Android driver's `#locks` pattern, sized to a single component instead of a whole device. * The map entry is removed once the download settles (success or failure), so a later, - * non-concurrent call starts a fresh attempt rather than replaying a stale result. + * non-concurrent call starts a fresh attempt rather than replaying a stale result. `componentId` + * is the runtime version being installed ("latest" for a bare `-downloadPlatform iOS`, the bare + * major version for the bounded-default case) -- reported on `component.install-*`, never + * parsed back out of `args`. */ - async #downloadRuntime(args: readonly string[]): Promise { + async #downloadRuntime(componentId: string, args: readonly string[]): Promise { const key = args.join(""); const inFlight = this.#downloadLocks.get(key); if (inFlight !== undefined) { return inFlight; } - const promise = this.#xcodebuildOrThrow(args).finally(() => { + const promise = this.#installComponent(componentId, args).finally(() => { if (this.#downloadLocks.get(key) === promise) { this.#downloadLocks.delete(key); } @@ -265,6 +287,33 @@ export class IosSimctlDriver implements Driver { return promise; } + /** + * Disk preflight, then `xcodebuild`, wrapped with `component.install-*` diagnostics. A + * preflight failure is reported before any diagnostic fires -- no install was actually + * attempted, so there is nothing to report as started or failed. + */ + async #installComponent(componentId: string, args: readonly string[]): Promise { + await assertDiskSpace(this.#filesystem, this.platform, IOS_RUNTIME_MIN_FREE_BYTES); + this.#onDiagnostic?.({ componentId, kind: "component-install-started" }); + const startedAt = this.#clock.now(); + try { + await this.#xcodebuildOrThrow(args); + } catch (error: unknown) { + this.#onDiagnostic?.({ + componentId, + durationMs: this.#clock.now() - startedAt, + error: stableError(error), + kind: "component-install-failed", + }); + throw error; + } + this.#onDiagnostic?.({ + componentId, + durationMs: this.#clock.now() - startedAt, + kind: "component-installed", + }); + } + async #xcodebuildOrThrow(args: readonly string[]): Promise { const result = await this.#processRunner.run("xcodebuild", args, { timeoutMs: this.#downloadTimeoutMs, @@ -883,3 +932,9 @@ function isRecord(value: unknown): value is Record { function errorMessage(error: unknown): string { return error instanceof Error ? error.message : String(error); } + +/** Matches `stableError` in `warm-pool-coordinator.ts` -- `device.purge-failed`'s own summary shape. */ +function stableError(error: unknown): string { + const value = error instanceof Error ? error : new Error(String(error)); + return `${value.name}: ${value.message}`; +} From 9d83c7bd3549e181c0e2954736bfd3782a02ae7f Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Tue, 1 Sep 2026 17:02:39 +0200 Subject: [PATCH 5/6] fix: address review findings on component installation (#67) Ten adversarially-verified review findings, fixed: 1. Extract a shared #mergeConfigIniLines helper for Android's config.ini read-modify-write (hardware properties + durable mark write), catching only missing-file errors and rethrowing everything else instead of silently clobbering config.ini on a real read failure. 2. Dedupe concurrent Android system-image installs behind a per-package in-flight promise map, mirroring the iOS driver's #downloadLocks. 3. Map InsufficientDiskSpaceError and the new platform-agnostic LicenseNotAcceptedError (AndroidLicenseNotAcceptedError now extends it, relocated next to InsufficientDiskSpaceError in core/driver.ts) to their own daemon error codes and CLI exit code 12; documented in docs/CLI.md. 4. iOS resolveSpec now also checks that an installed runtime's supportedDeviceTypeIds actually pairs with the requested model, not just that the version is in range. 5. RuntimeMissingError gained a `downloadable` flag (default true); the out-of-range, unpaired-runtime, and download-floor errors set it false so the daemon's "downloads are disabled by configuration" suffix never attaches to a request no download could have fixed. 6. Disk-space preflight now checks the volume a component actually installs to: the Android SDK root, and a new iOS coreSimulatorRoot option (wired from daemon/main.ts) instead of the daemon's own working directory. 7. Replaced the iOS 16.0 download-floor DriverCrashError with a typed RuntimeMissingError subclass (downloadable: false). 8. The bounded-default iOS runtime download path now rethrows InsufficientDiskSpaceError/RuntimeMissingError unchanged instead of wrapping them in a DriverCrashError. 9. Deduplicated the four copies of stableError into src/core/stable-error.ts, imported by both drivers and both core coordinators. 10. Covered by (1). --- docs/CLI.md | 2 + src/cli/index.test.ts | 2 + src/cli/index.ts | 2 + src/core/driver.ts | 29 +++++++ src/core/index.ts | 1 + src/core/quarantine-coordinator.ts | 6 +- src/core/stable-error.ts | 11 +++ src/core/warm-pool-coordinator.ts | 6 +- src/daemon/main.ts | 1 + src/daemon/server.test.ts | 82 ++++++++++++++++++- src/daemon/server.ts | 15 +++- src/drivers/android/index.test.ts | 115 +++++++++++++++++++++++++- src/drivers/android/index.ts | 120 ++++++++++++++++----------- src/drivers/ios/index.test.ts | 125 ++++++++++++++++++++++++++++- src/drivers/ios/index.ts | 78 +++++++++++++++--- src/ports/filesystem.ts | 15 +++- src/ports/index.ts | 7 +- 17 files changed, 539 insertions(+), 78 deletions(-) create mode 100644 src/core/stable-error.ts diff --git a/docs/CLI.md b/docs/CLI.md index bc3f4fc..11f3586 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -39,6 +39,8 @@ longer dumped to stderr on every failure, only on request via `--help`. | 12 | `NO_DRIVER` | no driver registered for the requested platform | | 12 | `RUNTIME_MISSING` | runtime not installed and no `--allow-download` | | 12 | `UNKNOWN_MODEL` | unknown device model for the platform | +| 12 | `INSUFFICIENT_DISK_SPACE` | not enough free disk space to install a component | +| 12 | `LICENSE_NOT_ACCEPTED` | a required license (e.g. an Android SDK license) is not accepted | | 13 | `REQUESTER_ALREADY_LEASED` | requester already holds a lease or has a pending request — one lease per agent in v1; release the named lease first | | 14 | — | `lease` held mode only: the daemon ended the lease without the holder asking (TTL backstop, operator `release`, or an unrecoverable device) | diff --git a/src/cli/index.test.ts b/src/cli/index.test.ts index 3b3758d..ebc3e89 100644 --- a/src/cli/index.test.ts +++ b/src/cli/index.test.ts @@ -73,6 +73,8 @@ describe("CLI boundary", () => { ["NO_CAPACITY", 11], ["RUNTIME_MISSING", 12], ["UNKNOWN_MODEL", 12], + ["INSUFFICIENT_DISK_SPACE", 12], + ["LICENSE_NOT_ACCEPTED", 12], ["BAD_REQUEST", 2], ["REQUESTER_ALREADY_LEASED", 13], ] as const)("maps %s daemon errors to exit %d", async (code, expected) => { diff --git a/src/cli/index.ts b/src/cli/index.ts index 4e8b62b..ed47ec2 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -47,6 +47,8 @@ const LEASE_LOST_EXIT_CODE = 14; const DAEMON_ERROR_EXIT_CODES: Readonly> = { BAD_FRAME: 2, BAD_REQUEST: 2, + INSUFFICIENT_DISK_SPACE: 12, + LICENSE_NOT_ACCEPTED: 12, NO_CAPACITY: 11, NO_DRIVER: 12, QUEUE_TIMEOUT: 10, diff --git a/src/core/driver.ts b/src/core/driver.ts index 0afcbe8..7b9d1d6 100644 --- a/src/core/driver.ts +++ b/src/core/driver.ts @@ -125,12 +125,24 @@ export interface Driver { } export class RuntimeMissingError extends Error { + /** + * Whether a download could plausibly fix this. `true` by default -- a plain "runtime not + * installed" is exactly what `--allow-download` exists for. A subclass reporting a request + * no download can ever satisfy (out of the model's pairing range, an installed runtime that + * does not pair with the model, a version older than Xcode's automatic-download floor) sets + * this `false` so callers (see the daemon's download-policy suffix) don't point someone at a + * flag that cannot help. + */ + readonly downloadable: boolean; + constructor( readonly platform: Platform, readonly osVersion: string, + options?: { readonly downloadable?: boolean }, ) { super(`Runtime missing for ${platform} ${osVersion}`); this.name = "RuntimeMissingError"; + this.downloadable = options?.downloadable ?? true; } } @@ -176,6 +188,23 @@ function formatGibibytes(bytes: number): string { return `${(bytes / 1024 ** 3).toFixed(1)} GiB`; } +/** + * A component install was refused because a required license/EULA is not accepted -- + * platform-agnostic the same way `RuntimeMissingError` is, so the daemon can map it to a + * stable error code without importing a driver module. `AndroidLicenseNotAcceptedError` (the + * only concrete case today) extends this with its own message; a future platform with the same + * shape of gate would do the same rather than the daemon special-casing Android. + */ +export class LicenseNotAcceptedError extends Error { + constructor( + readonly platform: Platform, + readonly componentName: string, + ) { + super(`A license required to install ${componentName} for ${platform} is not accepted`); + this.name = "LicenseNotAcceptedError"; + } +} + /** * Checked before a driver starts any multi-GB component download/install, so a full disk fails * fast with a clear message instead of filling up mid-download (see safety rule 4's spirit -- diff --git a/src/core/index.ts b/src/core/index.ts index 667d279..ae9367d 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -28,6 +28,7 @@ export { type DriverEstimate, type DriverReality, InsufficientDiskSpaceError, + LicenseNotAcceptedError, type ObservedDevice, type ObservedRunState, RuntimeMissingError, diff --git a/src/core/quarantine-coordinator.ts b/src/core/quarantine-coordinator.ts index 2e3aa30..2572f5a 100644 --- a/src/core/quarantine-coordinator.ts +++ b/src/core/quarantine-coordinator.ts @@ -3,6 +3,7 @@ import type { Clock, TimerHandle } from "../ports/index.js"; import type { DeviceRecord, Platform } from "./domain.js"; import type { Driver, DriverDevice } from "./driver.js"; import type { SerializedDecision } from "./serialized-decision.js"; +import { stableError } from "./stable-error.js"; export interface QuarantineDriverCatalog { get(platform: Platform): Driver; @@ -238,11 +239,6 @@ function backoffDelay(config: QuarantineRetryConfig, attemptsSoFar: number): num ); } -function stableError(error: unknown): string { - const value = error instanceof Error ? error : new Error(String(error)); - return `${value.name}: ${value.message}`; -} - /** * `address` is never trusted by a driver's `reclaim` / `destroy` -- they derive what they need * from `driverData` -- so a quarantined device that has no address yet is not a lie a driver diff --git a/src/core/stable-error.ts b/src/core/stable-error.ts new file mode 100644 index 0000000..1c35329 --- /dev/null +++ b/src/core/stable-error.ts @@ -0,0 +1,11 @@ +/** + * Renders any thrown value as `: ` -- the stable, human-readable summary + * shared by every `component.install-*` / `device.purge-failed`-style diagnostic that reports + * a caught error. Non-`Error` throws are wrapped first so every summary still has a name to + * report. One definition shared by both driver modules and the core coordinators that need it, + * rather than four copies drifting independently. + */ +export function stableError(error: unknown): string { + const value = error instanceof Error ? error : new Error(String(error)); + return `${value.name}: ${value.message}`; +} diff --git a/src/core/warm-pool-coordinator.ts b/src/core/warm-pool-coordinator.ts index 064577c..cf8dc7f 100644 --- a/src/core/warm-pool-coordinator.ts +++ b/src/core/warm-pool-coordinator.ts @@ -13,6 +13,7 @@ import type { Driver, DriverDevice } from "./driver.js"; import type { QuarantinePurgeFailure } from "./quarantine-coordinator.js"; import type { ReleasedLease } from "./registry.js"; import type { SerializedDecision } from "./serialized-decision.js"; +import { stableError } from "./stable-error.js"; export interface WarmPoolDriverCatalog { get(platform: Platform): Driver; @@ -211,11 +212,6 @@ function capacityDevices(devices: readonly DeviceRecord[]): readonly CapacityDev return devices.map((device) => ({ platform: device.spec.platform, state: device.state })); } -function stableError(error: unknown): string { - const value = error instanceof Error ? error : new Error(String(error)); - return `${value.name}: ${value.message}`; -} - /** * `address` is never trusted by a driver's `shutdown` / `reclaim` / `makeReady` -- they derive * whatever they need from `driverData` -- so a placeholder here is harmless. diff --git a/src/daemon/main.ts b/src/daemon/main.ts index 854fee8..5aa9079 100644 --- a/src/daemon/main.ts +++ b/src/daemon/main.ts @@ -215,6 +215,7 @@ export async function discoverDrivers(options: DriverDiscoveryContext): Promise< drivers.push( new IosSimctlDriver({ clock: options.clock, + coreSimulatorRoot: `${homedir()}/Library/Developer/CoreSimulator`, ...(options.downloadTimeoutMs === undefined ? {} : { downloadTimeoutMs: options.downloadTimeoutMs }), diff --git a/src/daemon/server.test.ts b/src/daemon/server.test.ts index 7983998..dfc379b 100644 --- a/src/daemon/server.test.ts +++ b/src/daemon/server.test.ts @@ -5,7 +5,16 @@ import { Socket, connect } from "node:net"; import { afterEach, describe, expect, it } from "vitest"; import { EventBus } from "../bus/index.js"; -import { type Config, CleanupReaper, FakeDriver, LeaseEngine, Registry } from "../core/index.js"; +import { + type Config, + CleanupReaper, + FakeDriver, + InsufficientDiskSpaceError, + LeaseEngine, + Registry, + RuntimeMissingError, +} from "../core/index.js"; +import { AndroidLicenseNotAcceptedError } from "../drivers/android/index.js"; import { FakeClock, FakeSystemStats, @@ -1138,6 +1147,77 @@ describe("DaemonServer download policy", () => { expect(response.error?.message).not.toContain("downloads.policy"); await client.close(); }); + + it("never attaches the download-policy suffix to an undownloadable RuntimeMissingError, even under the never policy", async () => { + const clock = new FakeClock(1_000); + const driver = new FakeDriver({ availableOsVersions: [], clock, platform: "ios" }); + // Stands in for a real out-of-range / unpaired-runtime error: no download could ever have + // fixed this request, so the policy is not what blocked it. + driver.failOn( + "resolveSpec", + 1, + new RuntimeMissingError("ios", "12.0", { downloadable: false }), + ); + const harness = await createHarness({ clock, downloads: { policy: "never" }, driver }); + const client = await createClient(harness.socketPath); + await hello(client); + + const response = await client.request("lease.request", { + allowDownload: true, + mode: "held", + requesterId: "agent-1", + request: { model: "iPhone 16", osVersion: "12.0", platform: "ios" }, + }); + + expect(response.ok).toBe(false); + expect(response.error).toMatchObject({ code: "RUNTIME_MISSING" }); + expect(response.error?.message).not.toContain("downloads.policy"); + await client.close(); + }); +}); + +describe("DaemonServer error code mapping", () => { + it("maps InsufficientDiskSpaceError to INSUFFICIENT_DISK_SPACE", async () => { + const clock = new FakeClock(1_000); + const driver = new FakeDriver({ availableOsVersions: [], clock, platform: "ios" }); + driver.failOn("resolveSpec", 1, new InsufficientDiskSpaceError("ios", 8 * 1024 ** 3, 0)); + const harness = await createHarness({ clock, downloads: { policy: "always" }, driver }); + const client = await createClient(harness.socketPath); + await hello(client); + + const response = await client.request("lease.request", { + mode: "held", + requesterId: "agent-1", + request: { model: "iPhone 16", osVersion: "26.5", platform: "ios" }, + }); + + expect(response.ok).toBe(false); + expect(response.error).toMatchObject({ code: "INSUFFICIENT_DISK_SPACE" }); + await client.close(); + }); + + it("maps LicenseNotAcceptedError to LICENSE_NOT_ACCEPTED", async () => { + const clock = new FakeClock(1_000); + const driver = new FakeDriver({ availableOsVersions: [], clock, platform: "android" }); + driver.failOn( + "resolveSpec", + 1, + new AndroidLicenseNotAcceptedError("system-images;android-35;google_apis;arm64-v8a"), + ); + const harness = await createHarness({ clock, downloads: { policy: "always" }, driver }); + const client = await createClient(harness.socketPath); + await hello(client); + + const response = await client.request("lease.request", { + mode: "held", + requesterId: "agent-1", + request: { model: "Pixel 8", osVersion: "35", platform: "android" }, + }); + + expect(response.ok).toBe(false); + expect(response.error).toMatchObject({ code: "LICENSE_NOT_ACCEPTED" }); + await client.close(); + }); }); // fallow-ignore-next-line complexity -- a test harness whose branches are all trivial optional-parameter defaulting. diff --git a/src/daemon/server.ts b/src/daemon/server.ts index c5078cc..daf642d 100644 --- a/src/daemon/server.ts +++ b/src/daemon/server.ts @@ -6,6 +6,8 @@ import { type LeaseProgress, type LeaseRecord, effectiveAllowDownload, + InsufficientDiskSpaceError, + LicenseNotAcceptedError, NoCapacityError, NoDriverError, QueueTimeoutError, @@ -576,8 +578,11 @@ export class DaemonServer { // The driver only ever sees the clamped-to-false permission, so its own // RuntimeMissingError just says "missing" -- it has no way to know a request asked for a // download and config refused it. Recover that distinction here, the one place that saw - // both sides, rather than teaching the driver about config. - if (blockedByDownloadPolicy && error instanceof RuntimeMissingError) { + // both sides, rather than teaching the driver about config. Gated on `downloadable`: a + // request no download could ever have fixed (out of range, an installed-but-unpaired + // runtime, older than the download floor) must not be blamed on the download policy -- + // that policy was never what stood between this request and success. + if (blockedByDownloadPolicy && error instanceof RuntimeMissingError && error.downloadable) { error.message = `${error.message} (downloads are disabled by configuration: downloads.policy is "never")`; } throw error; @@ -950,6 +955,12 @@ function errorCode(error: unknown): string { if (error instanceof UnknownModelError) { return "UNKNOWN_MODEL"; } + if (error instanceof InsufficientDiskSpaceError) { + return "INSUFFICIENT_DISK_SPACE"; + } + if (error instanceof LicenseNotAcceptedError) { + return "LICENSE_NOT_ACCEPTED"; + } if (error instanceof UnknownLeaseError) { return "UNKNOWN_LEASE"; } diff --git a/src/drivers/android/index.test.ts b/src/drivers/android/index.test.ts index a8d1e76..36fefdf 100644 --- a/src/drivers/android/index.test.ts +++ b/src/drivers/android/index.test.ts @@ -654,6 +654,44 @@ describe("AndroidDriver", () => { expect(config).toContain("hw.ramSize=2048"); }); + it("rethrows a non-missing-file config.ini read error from the mark write instead of clobbering it as empty", async () => { + const configPath = `${avdDirectory}/simlock_one.avd/config.ini`; + const originalConfig = "hw.ramSize=2048\n"; + const permissionError = Object.assign(new Error("EACCES: permission denied"), { + code: "EACCES", + }); + const failureFilesystem = new ReadFailureFilesystem(configPath, permissionError); + const filesystem = await androidFilesystem({ config: originalConfig }, failureFilesystem); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.avdmanager, [ + "create", + "avd", + "-n", + "simlock_one", + "-k", + /.+/, + "-d", + "pixel_8", + ]), + processResult(binaries.emulator, ["-version"], "Android emulator version 36.1.9"), + processResult(binaries.adb, ["devices"], "List of devices attached\n"), + ...baselineBuildExpectations({ launchArgs: ["-no-snapshot-load"] }), + markWriteExpectation("emulator-5554", "device-2"), + ]); + const driver = await createDriver(filesystem, runner, { ids: ["one"] }); + const spec = await driver.resolveSpec( + { model: "Pixel 8", osVersion: "34", platform: "android" }, + { allowDownload: false }, + ); + const device = await driver.provision(spec); + + await expect(driver.makeReady(device)).rejects.toBe(permissionError); + + failureFilesystem.armed = false; + await expect(filesystem.readFile(configPath)).resolves.toBe(originalConfig); + }); + it("rewrites the mark on reclaim's snapshot-restore success path", async () => { const harness = await provisionedHarness({ forBaselineReclaim: true }); await harness.driver.makeReady(harness.device); @@ -1077,6 +1115,29 @@ describe("AndroidDriver", () => { }); }); + it("dedupes concurrent resolveSpec calls for the same missing system image behind one sdkmanager install", async () => { + const filesystem = await androidFilesystem(); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.sdkmanager, [ + "--install", + "system-images;android-35;google_apis;arm64-v8a", + ]), + ]); + const driver = await createDriver(filesystem, runner); + const request = { model: "Pixel 8", osVersion: "35", platform: "android" } as const; + + const [first, second] = await Promise.all([ + driver.resolveSpec(request, { allowDownload: true }), + driver.resolveSpec(request, { allowDownload: true }), + ]); + + expect(first).toEqual({ model: "Pixel 8", osVersion: "35", platform: "android" }); + expect(second).toEqual({ model: "Pixel 8", osVersion: "35", platform: "android" }); + expect(runner.calls.filter((call) => call.args[0] === "--install")).toHaveLength(1); + }); + describe("component install diagnostics", () => { it("reports component-install-started then component-installed with a duration on a clean install", async () => { const filesystem = await androidFilesystem(); @@ -1225,6 +1286,34 @@ describe("AndroidDriver", () => { expect(runner.calls.some((call) => call.command === binaries.sdkmanager)).toBe(false); expect(diagnostics).toEqual([]); }); + + it("checks disk space on the SDK's own volume, not the daemon's working directory", async () => { + class RecordingFilesystem extends MemoryFilesystem { + readonly diskFreePaths: string[] = []; + + override async diskFree(path: string): Promise { + this.diskFreePaths.push(path); + return super.diskFree(path); + } + } + const recordingFilesystem = new RecordingFilesystem(); + const filesystem = await androidFilesystem({}, recordingFilesystem); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.sdkmanager, [ + "--install", + "system-images;android-35;google_apis;arm64-v8a", + ]), + ]); + const driver = await createDriver(filesystem, runner); + + await driver.resolveSpec( + { model: "Pixel 8", osVersion: "35", platform: "android" }, + { allowDownload: true }, + ); + + expect(recordingFilesystem.diskFreePaths).toEqual([sdk]); + }); }); describe("download timeout", () => { @@ -1464,14 +1553,38 @@ function customDeviceXml(name: string, ramMiB: number): string { ); } +/** + * Fails every `readFile` of `failingPath` with a non-ENOENT error while `armed`, so a test can + * assert a caller rethrows it instead of treating it as an absent file -- then disarm to read + * the path back and confirm nothing overwrote it in the meantime. + */ +class ReadFailureFilesystem extends MemoryFilesystem { + armed = true; + + constructor( + private readonly failingPath: string, + private readonly error: Error, + freeDiskBytes?: number, + ) { + super(freeDiskBytes); + } + + override async readFile(path: string): Promise { + if (this.armed && path === this.failingPath) { + throw this.error; + } + return super.readFile(path); + } +} + async function androidFilesystem( options: { readonly config?: string; readonly freeDiskBytes?: number; readonly images?: readonly (readonly [string, string, string])[]; } = {}, + filesystem: MemoryFilesystem = new MemoryFilesystem(options.freeDiskBytes), ): Promise { - const filesystem = new MemoryFilesystem(options.freeDiskBytes); for (const binary of Object.values(binaries)) { await filesystem.mkdirp(binary.slice(0, binary.lastIndexOf("/"))); await filesystem.writeFileAtomic(binary, "binary"); diff --git a/src/drivers/android/index.ts b/src/drivers/android/index.ts index 5db2a2c..7ec7516 100644 --- a/src/drivers/android/index.ts +++ b/src/drivers/android/index.ts @@ -9,19 +9,22 @@ import { DriverCrashError, type DriverEstimate, type DriverReality, + LicenseNotAcceptedError, type ObservedDevice, type ObservedMark, type ReclaimResult, RuntimeMissingError, } from "../../core/driver.js"; +import { stableError } from "../../core/stable-error.js"; import type { ComponentInstallDiagnostic } from "../diagnostics.js"; -import type { - Clock, - Filesystem, - IdGenerator, - ProcessHandle, - ProcessResult, - ProcessRunner, +import { + isMissingPathError, + type Clock, + type Filesystem, + type IdGenerator, + type ProcessHandle, + type ProcessResult, + type ProcessRunner, } from "../../ports/index.js"; import { isAndroidDriverData, type AndroidDriverData } from "./data.js"; import { @@ -116,13 +119,13 @@ export class SdkMissingError extends Error { } } -export class AndroidLicenseNotAcceptedError extends Error { +export class AndroidLicenseNotAcceptedError extends LicenseNotAcceptedError { constructor(readonly packageName: string) { - super( + super("android", packageName); + this.message = `sdkmanager refused to install ${packageName}: an Android SDK license is not accepted. ` + - `Set "downloads.acceptAndroidLicenses": true in config to accept automatically, or run ` + - `\`sdkmanager --licenses\` manually.`, - ); + `Set "downloads.acceptAndroidLicenses": true in config to accept automatically, or run ` + + `\`sdkmanager --licenses\` manually.`; this.name = "AndroidLicenseNotAcceptedError"; } } @@ -163,6 +166,7 @@ export class AndroidDriver implements Driver { readonly #filesystem: Filesystem; readonly #hostAbi: string; readonly #idGenerator: IdGenerator; + readonly #installLocks = new Map>(); readonly #locks = new Map>(); readonly #onDiagnostic: ((diagnostic: AndroidDriverDiagnostic) => void) | undefined; readonly #portAllocator: PortAllocator; @@ -587,32 +591,34 @@ export class AndroidDriver implements Driver { return first.id; } - /** - * Merges `properties` into the AVD's `config.ini`, overwriting any key it already has and - * appending the rest -- same read-modify-write shape as `#writeDurableMark` below. - */ + /** Merges `properties` into the AVD's `config.ini` -- see `#mergeConfigIniLines`. */ async #applyHardwareProperties( avdName: string, properties: Readonly>, ): Promise { - const path = this.#configIniPath(avdName); - let contents: string; - try { - contents = await this.#filesystem.readFile(path); - } catch { - contents = ""; + await this.#mergeConfigIniLines(avdName, properties); + } + + /** + * Dedupes concurrent installs of the same system-image package behind one in-flight promise + * -- mirrors the iOS driver's `#downloadLocks`, sized to a package instead of a whole + * `xcodebuild` invocation. The map entry is removed once the install settles (success or + * failure), so a later, non-concurrent call starts a fresh attempt rather than replaying a + * stale result. + */ + async #installSystemImage(packageName: string): Promise { + const inFlight = this.#installLocks.get(packageName); + if (inFlight !== undefined) { + return inFlight; } - const lines = contents === "" ? [] : contents.replace(/\r?\n$/, "").split(/\r?\n/); - for (const [key, value] of Object.entries(properties)) { - const line = `${key}=${value}`; - const existingIndex = lines.findIndex((entry) => entry.startsWith(`${key}=`)); - if (existingIndex >= 0) { - lines[existingIndex] = line; - } else { - lines.push(line); + + const promise = this.#installSystemImageOnce(packageName).finally(() => { + if (this.#installLocks.get(packageName) === promise) { + this.#installLocks.delete(packageName); } - } - await this.#filesystem.writeFileAtomic(path, `${lines.join("\n")}\n`); + }); + this.#installLocks.set(packageName, promise); + return promise; } /** @@ -624,8 +630,13 @@ export class AndroidDriver implements Driver { * sees exactly one `install-failed` regardless of which branch below throws, never one per * attempt. */ - async #installSystemImage(packageName: string): Promise { - await assertDiskSpace(this.#filesystem, this.platform, ANDROID_SYSTEM_IMAGE_MIN_FREE_BYTES); + async #installSystemImageOnce(packageName: string): Promise { + await assertDiskSpace( + this.#filesystem, + this.platform, + ANDROID_SYSTEM_IMAGE_MIN_FREE_BYTES, + this.#sdk.root, + ); this.#onDiagnostic?.({ componentId: packageName, kind: "component-install-started" }); const startedAt = this.#clock.now(); try { @@ -796,20 +807,41 @@ export class AndroidDriver implements Driver { } async #writeDurableMark(avdName: string, token: string): Promise { + await this.#mergeConfigIniLines(avdName, { [DURABLE_MARK_KEY]: token }); + } + + /** + * Reads `avdName`'s `config.ini`, merges `entries` into it -- overwriting any key already + * present, appending the rest -- and writes it back atomically. Shared by + * `#applyHardwareProperties` and `#writeDurableMark`, the driver's two config.ini + * read-modify-write sites. A missing file (the AVD's config.ini not created yet) starts the + * merge from empty content; any other read failure is rethrown rather than treated as an + * empty file -- silently starting from "" on, say, an EACCES or EIO would write back only + * `entries` and clobber whatever config.ini already held. + */ + async #mergeConfigIniLines( + avdName: string, + entries: Readonly>, + ): Promise { const path = this.#configIniPath(avdName); let contents: string; try { contents = await this.#filesystem.readFile(path); - } catch { + } catch (error: unknown) { + if (!isMissingPathError(error)) { + throw error; + } contents = ""; } const lines = contents === "" ? [] : contents.replace(/\r?\n$/, "").split(/\r?\n/); - const markLine = `${DURABLE_MARK_KEY}=${token}`; - const existingIndex = lines.findIndex((line) => line.startsWith(`${DURABLE_MARK_KEY}=`)); - if (existingIndex >= 0) { - lines[existingIndex] = markLine; - } else { - lines.push(markLine); + for (const [key, value] of Object.entries(entries)) { + const line = `${key}=${value}`; + const existingIndex = lines.findIndex((entry) => entry.startsWith(`${key}=`)); + if (existingIndex >= 0) { + lines[existingIndex] = line; + } else { + lines.push(line); + } } await this.#filesystem.writeFileAtomic(path, `${lines.join("\n")}\n`); } @@ -1324,12 +1356,6 @@ function hasUnacceptedLicense(result: ProcessResult): boolean { return /licen[cs]e/i.test(combined) && /not accepted/i.test(combined); } -/** Matches `stableError` in `warm-pool-coordinator.ts` -- `device.purge-failed`'s own summary shape. */ -function stableError(error: unknown): string { - const value = error instanceof Error ? error : new Error(String(error)); - return `${value.name}: ${value.message}`; -} - /** * `[builtin, user]`: `avdmanager list device` first, then a read-only parse of Android * Studio's `~/.android/devices.xml`. `ANDROID_SDK_HOME` (not `ANDROID_AVD_HOME`, which only diff --git a/src/drivers/ios/index.test.ts b/src/drivers/ios/index.test.ts index 51bd896..d3a9b1b 100644 --- a/src/drivers/ios/index.test.ts +++ b/src/drivers/ios/index.test.ts @@ -174,12 +174,54 @@ describe("IosSimctlDriver", () => { expect(result).toBeInstanceOf(RuntimeMissingError); expect(result).toMatchObject({ + downloadable: false, message: expect.stringContaining("iPhone Xs supports iOS 12.0-18.6"), }); // No xcodebuild (or any further simctl) call: out-of-range is checked before download logic. expect(runner.calls).toHaveLength(1); }); + it("rejects an installed runtime whose supportedDeviceTypes omits the requested model, without ever calling simctl create", async () => { + const catalog = JSON.stringify({ + devicetypes: [ + { + identifier: "com.apple.CoreSimulator.SimDeviceType.iPhone-16", + maxRuntimeVersion: 16_777_215, + minRuntimeVersion: 0, + name: "iPhone 16", + }, + ], + runtimes: [ + { + identifier: "com.apple.CoreSimulator.SimRuntime.iOS-18-4", + isAvailable: true, + name: "iOS 18.4", + supportedDeviceTypes: [], + version: "18.4", + }, + ], + }); + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: catalog } }, + ]); + const driver = createDriver(runner); + + const result = await driver + .resolveSpec( + { model: "iPhone 16", osVersion: "18.4", platform: "ios" }, + { allowDownload: true }, + ) + .catch((error: unknown) => error); + + expect(result).toBeInstanceOf(RuntimeMissingError); + expect(result).toMatchObject({ + downloadable: false, + message: "iOS 18.4 is installed but does not support iPhone 16", + }); + // Installed and in range, but not paired: no download attempted, no simctl create. + expect(runner.calls).toHaveLength(1); + }); + it("selects the newest installed runtime that actually pairs with the model, not the newest overall", async () => { const runner = new ScriptedProcessRunner([ { match: listInvocation, result: { code: 0, stderr: "", stdout: pairingFixture } }, @@ -205,8 +247,8 @@ describe("IosSimctlDriver", () => { ) .catch((error: unknown) => error); - expect(result).toBeInstanceOf(DriverCrashError); - expect(result).toMatchObject({ message: expect.stringContaining("16.0") }); + expect(result).toBeInstanceOf(RuntimeMissingError); + expect(result).toMatchObject({ downloadable: false, message: expect.stringContaining("16.0") }); // Range check passed (13.0 is within iPhone 7's 9.0-15.0), but no xcodebuild call was made. expect(runner.calls).toHaveLength(1); }); @@ -388,6 +430,85 @@ describe("IosSimctlDriver", () => { expect(runner.calls.map((call) => call.command)).toEqual(["xcrun"]); expect(diagnostics).toEqual([]); }); + + it("surfaces a disk-preflight failure from the bounded-default download path as InsufficientDiskSpaceError, not DriverCrashError", async () => { + // A device type with a finite max (bounded, unlike the "latest" test above) and no + // installed runtime pairs with it -- forces the bounded-default download branch, whose + // catch previously wrapped every failure (including this one) in a DriverCrashError. The + // installed runtime below keeps the catalog non-empty (required to parse at all) without + // pairing with the requested model. + const catalog = JSON.stringify({ + devicetypes: [ + { + identifier: "com.apple.CoreSimulator.SimDeviceType.iPhone-Xs", + maxRuntimeVersion: 1_181_184, + minRuntimeVersion: 786_432, + name: "iPhone Xs", + }, + ], + runtimes: [ + { + identifier: "com.apple.CoreSimulator.SimRuntime.iOS-18-4", + isAvailable: true, + name: "iOS 18.4", + supportedDeviceTypes: [], + version: "18.4", + }, + ], + }); + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: catalog } }, + ]); + const filesystem = new MemoryFilesystem(1024); + const driver = createDriver(runner, new FakeClock(), filesystem); + + const error = await driver + .resolveSpec({ model: "iPhone Xs", platform: "ios" }, { allowDownload: true }) + .catch((caught: unknown) => caught); + + expect(error).toBeInstanceOf(InsufficientDiskSpaceError); + // Only the initial catalog list happened: no xcodebuild invocation attempted. + expect(runner.calls.map((call) => call.command)).toEqual(["xcrun"]); + }); + + it("checks disk space on the configured CoreSimulator volume, not the daemon's own working directory", async () => { + class RecordingFilesystem extends MemoryFilesystem { + readonly diskFreePaths: string[] = []; + + override async diskFree(path: string): Promise { + this.diskFreePaths.push(path); + return super.diskFree(path); + } + } + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixture } }, + { + match: { + command: "xcodebuild", + args: ["-downloadPlatform", "iOS", "-buildVersion", "18.6"], + }, + }, + { + match: listInvocation, + result: { code: 0, stderr: "", stdout: listFixtureAfterDownload }, + }, + ]); + const filesystem = new RecordingFilesystem(); + const driver = new IosSimctlDriver({ + clock: new FakeClock(), + coreSimulatorRoot: "/Users/agent/Library/Developer/CoreSimulator", + filesystem, + idGenerator: { generate: () => "device-1" }, + processRunner: runner, + }); + + await driver.resolveSpec( + { model: "iPhone 16", osVersion: "18.6", platform: "ios" }, + { allowDownload: true }, + ); + + expect(filesystem.diskFreePaths).toEqual(["/Users/agent/Library/Developer/CoreSimulator"]); + }); }); it("provisions with the exact simctl argv and returns opaque iOS driver data", async () => { diff --git a/src/drivers/ios/index.ts b/src/drivers/ios/index.ts index 3d0cd68..895968c 100644 --- a/src/drivers/ios/index.ts +++ b/src/drivers/ios/index.ts @@ -8,6 +8,7 @@ import { DriverCrashError, type DriverEstimate, type DriverReality, + InsufficientDiskSpaceError, type ObservedDevice, type ObservedRunState, RuntimeMissingError, @@ -15,6 +16,7 @@ import { } from "../../core/index.js"; import type { ObservedMark } from "../../core/driver.js"; import type { DeviceSpec } from "../../core/index.js"; +import { stableError } from "../../core/stable-error.js"; import type { Clock, Filesystem, @@ -62,6 +64,14 @@ interface IosDriverData { export interface IosSimctlDriverOptions { readonly clock: Clock; + /** + * Volume a runtime download actually lands on, for the disk preflight in + * `#installComponent` -- simulator runtimes install under `~/Library/Developer/ + * CoreSimulator`, which is not necessarily the same volume as the daemon's working + * directory. Defaults to `"."` (the daemon process's own volume) only when nothing better + * is available, mirroring the Android driver's use of `sdk.root`. + */ + readonly coreSimulatorRoot?: string; /** Per-download timeout; defaults to `downloads.timeoutMs`'s own default. */ readonly downloadTimeoutMs?: number; readonly filesystem: Filesystem; @@ -112,6 +122,7 @@ type ProcessOutcome = export class IosSimctlDriver implements Driver { readonly platform = "ios" as const; readonly #clock: Clock; + readonly #coreSimulatorRoot: string; readonly #downloadLocks = new Map>(); readonly #downloadTimeoutMs: number; readonly #filesystem: Filesystem; @@ -123,6 +134,7 @@ export class IosSimctlDriver implements Driver { constructor(options: IosSimctlDriverOptions) { this.#clock = options.clock; + this.#coreSimulatorRoot = options.coreSimulatorRoot ?? "."; this.#downloadTimeoutMs = options.downloadTimeoutMs ?? DEFAULT_DOWNLOAD_TIMEOUT_MS; this.#filesystem = options.filesystem; this.#idGenerator = options.idGenerator; @@ -166,6 +178,13 @@ export class IosSimctlDriver implements Driver { const installed = findInstalledRuntime(catalog, osVersion); if (installed !== undefined) { + // In the model's declared `[min, max]` range is necessary but not sufficient: a runtime + // can be installed and still not pair with this specific device type (`supportedDeviceTypeIds` + // is the authoritative source once a runtime is actually on disk -- the range above is + // only a static hint). Checked before committing, so a mismatch never reaches `simctl create`. + if (!installed.supportedDeviceTypeIds.has(deviceType.identifier)) { + throw new IosRuntimeUnpairedError(deviceType.name, osVersion); + } return this.#commitResolution(deviceType, installed); } @@ -178,10 +197,7 @@ export class IosSimctlDriver implements Driver { } if (isTooOldToDownload(osVersion)) { - throw new DriverCrashError( - `iOS ${osVersion} predates Xcode's automatic download support (introduced for iOS ` + - `16.0 and newer); install it manually via Xcode`, - ); + throw new IosDownloadFloorError(osVersion); } await this.#downloadRuntime(osVersion, [ @@ -233,6 +249,13 @@ export class IosSimctlDriver implements Driver { try { await this.#downloadRuntime(major, ["-downloadPlatform", "iOS", "-buildVersion", major]); } catch (error: unknown) { + // A disk preflight failure or a typed "nothing to do here" (e.g. a concurrent caller's + // RuntimeMissingError) is meaningful on its own and must reach the caller unchanged -- + // wrapping it in a DriverCrashError below would bury a clean, actionable error under an + // opaque "could not download" one. + if (error instanceof InsufficientDiskSpaceError || error instanceof RuntimeMissingError) { + throw error; + } throw new DriverCrashError( `Could not download a default iOS runtime for ${deviceType.name} (tried ${major}): ` + `${errorMessage(error)}; pass --os to request an exact release`, @@ -293,7 +316,12 @@ export class IosSimctlDriver implements Driver { * attempted, so there is nothing to report as started or failed. */ async #installComponent(componentId: string, args: readonly string[]): Promise { - await assertDiskSpace(this.#filesystem, this.platform, IOS_RUNTIME_MIN_FREE_BYTES); + await assertDiskSpace( + this.#filesystem, + this.platform, + IOS_RUNTIME_MIN_FREE_BYTES, + this.#coreSimulatorRoot, + ); this.#onDiagnostic?.({ componentId, kind: "component-install-started" }); const startedAt = this.#clock.now(); try { @@ -687,12 +715,44 @@ class IosUnknownModelError extends UnknownModelError { */ class IosVersionOutOfRangeError extends RuntimeMissingError { constructor(model: string, requested: string, deviceType: DeviceType) { - super("ios", requested); + super("ios", requested, { downloadable: false }); const range = formatVersionRange(deviceType.minRuntimeVersion, deviceType.maxRuntimeVersion); this.message = `${model} supports iOS ${range}; iOS ${requested} is out of range`; } } +/** + * The requested runtime is installed and its version falls in the model's declared range, but + * the runtime's own `supportedDeviceTypes` (authoritative once it is actually on disk) does not + * include this model -- e.g. a device Apple dropped support for in a later point release of an + * OS it otherwise still ships. Extends `RuntimeMissingError` for the same reason + * `IosVersionOutOfRangeError` does (shared error code / exit status), and sets `downloadable: + * false` for the same reason: the runtime is already installed, so downloading it again changes + * nothing. + */ +class IosRuntimeUnpairedError extends RuntimeMissingError { + constructor(model: string, requested: string) { + super("ios", requested, { downloadable: false }); + this.message = `iOS ${requested} is installed but does not support ${model}`; + } +} + +/** + * A requested version predates Xcode's automatic download support (`xcodebuild + * -downloadPlatform` only reaches back to iOS 16.0 -- see `IOS_DOWNLOAD_FLOOR`). Not a driver + * crash: nothing went wrong, the request is simply outside what `--allow-download` can ever do. + * `RuntimeMissingError` with `downloadable: false` reports that distinction the same way the + * out-of-range and unpaired-runtime errors do, rather than surfacing as an opaque internal error. + */ +class IosDownloadFloorError extends RuntimeMissingError { + constructor(requested: string) { + super("ios", requested, { downloadable: false }); + this.message = + `iOS ${requested} predates Xcode's automatic download support (introduced for iOS ` + + `16.0 and newer); install it manually via Xcode`; + } +} + interface ParsedManagedDevice { readonly dataPath: string | undefined; readonly name: string; @@ -932,9 +992,3 @@ function isRecord(value: unknown): value is Record { function errorMessage(error: unknown): string { return error instanceof Error ? error.message : String(error); } - -/** Matches `stableError` in `warm-pool-coordinator.ts` -- `device.purge-failed`'s own summary shape. */ -function stableError(error: unknown): string { - const value = error instanceof Error ? error : new Error(String(error)); - return `${value.name}: ${value.message}`; -} diff --git a/src/ports/filesystem.ts b/src/ports/filesystem.ts index 7ac0dfb..e925a60 100644 --- a/src/ports/filesystem.ts +++ b/src/ports/filesystem.ts @@ -175,7 +175,10 @@ export class MemoryFilesystem implements Filesystem { #entryAt(path: string): MemoryEntry { const entry = this.#entries.get(path); if (entry === undefined) { - throw new Error(`No such file or directory: ${path}`); + // Carries `code: "ENOENT"` like Node's real fs errors, so callers that branch on + // `isMissingPathError` (rather than swallowing every read failure) behave the same + // against this fake as they do against `NodeFilesystem`. + throw enoent(path); } return entry; @@ -194,6 +197,14 @@ function parentPath(path: string): string { return lastSeparator <= 0 ? "/" : path.slice(0, lastSeparator); } -function isMissingPathError(error: unknown): error is NodeJS.ErrnoException { +export function isMissingPathError(error: unknown): error is NodeJS.ErrnoException { return typeof error === "object" && error !== null && "code" in error && error.code === "ENOENT"; } + +function enoent(path: string): NodeJS.ErrnoException { + const error = new Error( + `ENOENT: no such file or directory, open '${path}'`, + ) as NodeJS.ErrnoException; + error.code = "ENOENT"; + return error; +} diff --git a/src/ports/index.ts b/src/ports/index.ts index ff9cec2..af85faa 100644 --- a/src/ports/index.ts +++ b/src/ports/index.ts @@ -1,4 +1,9 @@ -export { type Filesystem, MemoryFilesystem, NodeFilesystem } from "./filesystem.js"; +export { + type Filesystem, + isMissingPathError, + MemoryFilesystem, + NodeFilesystem, +} from "./filesystem.js"; export { resolveSimlockHome } from "./paths.js"; export { type DaemonLauncher, FakeDaemonLauncher, NodeDaemonLauncher } from "./daemon-launcher.js"; export { From a9a4ffd524be903263d491427650f0b32ea93e14 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Tue, 1 Sep 2026 18:32:22 +0200 Subject: [PATCH 6/6] fix: harden component installation after external review (#67) Nine adversarially-verified review findings, fixed: 1. The daemon's "downloads are disabled by configuration: downloads.policy is \"never\"" suffix now attaches to any downloadable RuntimeMissingError under the never policy, regardless of whether the individual request asked for a download -- the driver's own message suggests --allow-download, which can never help under never, so the suffix is the correction every caller needs, not just the ones that asked. 2. iOS's parseCatalog only requires a non-empty devicetypes list now; an empty runtimes list (a fresh Xcode with nothing downloaded yet) no longer throws before resolveSpec can reach the download-latest path. All catalog.runtimes consumers (findInstalledRuntime, pairedInstalledRuntime, listCatalog) already tolerated an empty array. 3. #resolveExactRuntime's post-download commit now also checks the refreshed runtime's supportedDeviceTypeIds against the requested device type, throwing IosRuntimeUnpairedError on a mismatch instead of committing a downloaded-but-unpaired runtime -- mirrors the already-installed pairing check just above it. 4. device-profile-source.ts's parseDevicesXml now rejects (throws, surfaced through the existing malformed-devices.xml diagnostic path) any device name or property value containing CR, LF, or NUL, closing a config.ini injection route through a crafted devices.xml. The Android driver's #mergeConfigIniLines gained its own independent defense-in-depth guard rejecting any key/value with an embedded line break. 5. Android's hasUnacceptedLicense regex now also matches "licenses have not been accepted." (not just "... not accepted."), the second documented sdkmanager phrasing the comment already claimed to cover. 6. component.installed is now a verified fact in both drivers: it fires only after a post-install re-scan (iOS: simctl catalog re-load + pairing check; Android: system-images tree re-scan) confirms the component the request actually needed is present. An installer that exits 0 but leaves nothing behind now reports component-install-failed and throws (DriverCrashError / IosRuntimeUnpairedError) instead of claiming success. 7. NodeProcessRunner's timeout handling now escalates: a fixed 10s after the timeout-triggered SIGTERM, it sends SIGKILL if the child hasn't exited, so a child that ignores SIGTERM (or is itself hung) can no longer hold run() open indefinitely. 8. Added core.DiskSpaceGuard: reserves free space minus other outstanding reservations (keyed per path), returning a release function, so concurrent installs across drivers can't each pass an instantaneous disk-free check and jointly overfill the volume. One shared instance is constructed in src/daemon/main.ts and injected into both drivers, replacing their bare assertDiskSpace preflight calls. 9. Driver.resolveSpec's options gained an optional requesterId, threaded from LeaseAcquisitionCoordinator#resolveAndDrive through both drivers' component-install diagnostics into the bridged component.install-* event payloads and the daemon's durable "Component installed" log line, so an install is attributable to the request that caused it. Also updates docs/ARCHITECTURE.md (verified-fact event timing, the shared DiskSpaceGuard, requesterId attribution, empty-runtime-catalog resolution) and docs/EVENTS.md's payload column for the three component events. Verified: pnpm run typecheck && pnpm run lint && pnpm run test all green. --- docs/ARCHITECTURE.md | 54 ++- docs/EVENTS.md | 6 +- e2e/fake-driver/fake-driver.ts | 2 +- src/bus/index.ts | 9 +- src/core/driver-catalog.ts | 2 +- src/core/driver.test.ts | 110 ++++++ src/core/driver.ts | 72 +++- src/core/fake-driver.ts | 2 +- src/core/index.ts | 2 +- src/core/lease-acquisition-coordinator.ts | 5 + src/daemon/main.test.ts | 50 +++ src/daemon/main.ts | 38 +- src/daemon/server.test.ts | 24 ++ src/daemon/server.ts | 23 +- .../android/device-profile-source.test.ts | 47 +++ src/drivers/android/device-profile-source.ts | 33 ++ src/drivers/android/index.test.ts | 361 +++++++++++++++--- src/drivers/android/index.ts | 125 ++++-- src/drivers/diagnostics.ts | 9 +- src/drivers/ios/index.test.ts | 205 ++++++++++ src/drivers/ios/index.ts | 183 +++++++-- src/ports/index.ts | 2 +- src/ports/process-runner.test.ts | 19 + src/ports/process-runner.ts | 23 ++ 24 files changed, 1258 insertions(+), 148 deletions(-) create mode 100644 src/core/driver.test.ts diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 3e40abc..aa36b0d 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -472,14 +472,40 @@ wires each driver's `onDiagnostic` at construction time [EVENTS.md](EVENTS.md#components). This is also why those events are attributed to the `driver-diagnostics` emitter rather than to `IosSimctlDriver` or `AndroidDriver` directly: the driver only observed the fact, the daemon -layer is what committed it to the bus. - -Before starting either install, the driver checks free disk space against a +layer is what committed it to the bus. Both drivers also thread the +requesting lease's `requesterId` (when `resolveSpec`'s caller knew one — see +`LeaseAcquisitionCoordinator#resolveAndDrive`) through the diagnostic into +the bridged event's payload, so a component install is attributable to the +request that caused it. + +`component.installed` is a verified fact, not "the installer exited 0": +`xcodebuild`/`sdkmanager` reporting success only means the tool claims to +have finished, not that the thing the request actually needed — a runtime at +the requested version, paired with the requested device type for iOS; the +requested system image for Android — is now present. Both drivers re-scan +their own catalog (`simctl list` / the SDK's `system-images` tree) after the +installer returns and only report `component.installed` once that re-scan +confirms it; a re-scan that comes up empty reports `component.install-failed` +with that reason instead, and the caller still sees the same typed error it +always did (`DriverCrashError` for iOS's "still not installed" case, +`IosRuntimeUnpairedError` for a downloaded-but-unpaired runtime). Exactly one +terminal fact fires per install attempt, matching the pre-existing +`component.install-started` timing. + +Before starting either install, the driver reserves free disk space against a conservative per-component estimate (~8 GiB for an iOS runtime, ~2 GiB for an -Android system image) via `Filesystem#diskFree` and fails fast with a typed -`InsufficientDiskSpaceError` naming required vs. available bytes — no -`component.install-*` diagnostic fires for a preflight failure, since no -install was actually attempted. +Android system image) through a `DiskSpaceGuard` shared across every driver +(`src/daemon/main.ts` constructs one instance and passes it to each driver's +options) rather than a bare instantaneous `Filesystem#diskFree` reading: two +concurrent installs — an iOS runtime download racing an Android system-image +install, or two of either — could otherwise each observe enough free space +individually and jointly overfill the volume neither alone would have. The +guard tracks bytes reserved but not yet released, keyed per path, and checks +free space *minus* those outstanding reservations; the reservation is +released once the install settles either way. A reservation that doesn't fit +still fails fast with the same typed `InsufficientDiskSpaceError` naming +required vs. available bytes, and no `component.install-*` diagnostic fires +for a preflight failure, since no install was actually attempted. ## External APIs behind interfaces (ports) @@ -590,7 +616,9 @@ installed on an agent's behalf stays attributable in `daemon.log` after the event ring buffer resets on restart — the same durable-vs-ring-buffer split as everything else in this section, applied to component installs specifically because there is no registry entry or uninstall for them to be recovered from -otherwise (see "Out of scope" in the #67 issue). +otherwise (see "Out of scope" in the #67 issue). The log line carries +`requesterId` whenever the event payload has one, so the durable record names +which agent's request caused the install, not just that one happened. ## Device requests @@ -601,9 +629,13 @@ installed runtime that both falls inside the device type's supported range (`simctl list devicetypes`' `minRuntimeVersion`/`maxRuntimeVersion`) and still lists the model in its `supportedDeviceTypes`, not the newest installed runtime overall (a newer runtime can drop a model, as iOS 26 did -for iPhone XS/XR). If the requested runtime / system image is not -installed, the lease fails with a clear error unless downloads are -permitted for that request (downloads are multi-GB and must never be +for iPhone XS/XR). This still resolves on a fresh Xcode install with zero +simulator runtimes present at all — an empty runtime list is a normal +starting state, not a malformed catalog, so it falls straight through to +the same "not installed, permitted to download" path as a non-empty catalog +that simply lacks a matching runtime. If the requested runtime / system +image is not installed, the lease fails with a clear error unless downloads +are permitted for that request (downloads are multi-GB and must never be triggered implicitly). An OS version outside a model's supported range fails immediately with the range named in the error — never as an attempted download, since no download could make it work. diff --git a/docs/EVENTS.md b/docs/EVENTS.md index 7b858c3..24cc84b 100644 --- a/docs/EVENTS.md +++ b/docs/EVENTS.md @@ -44,9 +44,9 @@ in short: `subject.past-tense-fact`, emitted post-commit, facts not commands. | Event | Payload (key fields) | Emitted when | Emitter | Status | |---|---|---|---|---| -| `component.install-started` | platform, component id (iOS runtime version or "latest"; Android `sdkmanager` package name) | a driver is about to run `xcodebuild -downloadPlatform` / `sdkmanager --install` for a missing component, disk preflight already passed | driver-diagnostics | implemented | -| `component.installed` | platform, component id, duration | the install succeeded | driver-diagnostics | implemented | -| `component.install-failed` | platform, component id, duration, stable error summary | the install failed, including a license-retry failure (exactly one `install-failed` per attempted install, never one per retry) | driver-diagnostics | implemented | +| `component.install-started` | platform, component id (iOS runtime version or "latest"; Android `sdkmanager` package name), requester id (when the triggering resolution knew one) | a driver is about to run `xcodebuild -downloadPlatform` / `sdkmanager --install` for a missing component, disk preflight already passed | driver-diagnostics | implemented | +| `component.installed` | platform, component id, duration, requester id | the install succeeded **and** a post-install re-scan confirmed the component the request actually needed is present (paired with the requested device type, for iOS) — never fired on a bare exit-0 | driver-diagnostics | implemented | +| `component.install-failed` | platform, component id, duration, stable error summary, requester id | the install failed, including a license-retry failure (exactly one `install-failed` per attempted install, never one per retry), **or** the installer exited 0 but the post-install re-scan could not confirm the component | driver-diagnostics | implemented | Drivers never touch the event bus directly (architecture rule 5): both drivers report these facts through their own `onDiagnostic` callback (mirroring the Android driver's pre-existing diff --git a/e2e/fake-driver/fake-driver.ts b/e2e/fake-driver/fake-driver.ts index 5e52492..1f8bf40 100644 --- a/e2e/fake-driver/fake-driver.ts +++ b/e2e/fake-driver/fake-driver.ts @@ -69,7 +69,7 @@ export class OutOfProcessFakeDriver implements Driver { async resolveSpec( request: DeviceRequest, - options: { readonly allowDownload: boolean }, + options: { readonly allowDownload: boolean; readonly requesterId?: string }, ): Promise { const script = await this.#beforeCall("resolveSpec", [request, options]); this.#assertKnownModel(request.model, script); diff --git a/src/bus/index.ts b/src/bus/index.ts index 11009f1..65606ff 100644 --- a/src/bus/index.ts +++ b/src/bus/index.ts @@ -86,17 +86,24 @@ export interface EventMap { }; "device.shutdown": { readonly deviceId: string; readonly initiator: string }; "device.deleted": { readonly deviceId: string; readonly initiator: string }; - "component.install-started": { readonly platform: string; readonly componentId: string }; + "component.install-started": { + readonly platform: string; + readonly componentId: string; + /** The requester on whose behalf this install runs, when the triggering resolution knew one. */ + readonly requesterId?: string; + }; "component.installed": { readonly platform: string; readonly componentId: string; readonly durationMs: number; + readonly requesterId?: string; }; "component.install-failed": { readonly platform: string; readonly componentId: string; readonly durationMs: number; readonly error: string; + readonly requesterId?: string; }; "daemon.started": { readonly version: string; readonly configSnapshot: unknown }; "daemon.stopping": { readonly reason: string }; diff --git a/src/core/driver-catalog.ts b/src/core/driver-catalog.ts index 9f1a06a..dc0f346 100644 --- a/src/core/driver-catalog.ts +++ b/src/core/driver-catalog.ts @@ -30,7 +30,7 @@ export class DriverCatalog { async resolveSpec( request: DeviceRequest, - options: { readonly allowDownload: boolean }, + options: { readonly allowDownload: boolean; readonly requesterId?: string }, ): Promise { return this.get(request.platform).resolveSpec(request, options); } diff --git a/src/core/driver.test.ts b/src/core/driver.test.ts new file mode 100644 index 0000000..cb5ce2e --- /dev/null +++ b/src/core/driver.test.ts @@ -0,0 +1,110 @@ +import { describe, expect, it } from "vitest"; + +import { MemoryFilesystem } from "../ports/index.js"; +import { assertDiskSpace, DiskSpaceGuard, InsufficientDiskSpaceError } from "./driver.js"; + +const gibibyte = 1024 ** 3; + +describe("assertDiskSpace", () => { + it("resolves when free space covers the requirement", async () => { + const filesystem = new MemoryFilesystem(10 * gibibyte); + + await expect( + assertDiskSpace(filesystem, "ios", 6 * gibibyte, "/volume"), + ).resolves.toBeUndefined(); + }); + + it("throws InsufficientDiskSpaceError naming required and available bytes when it doesn't", async () => { + const filesystem = new MemoryFilesystem(4 * gibibyte); + + const error = await assertDiskSpace(filesystem, "android", 6 * gibibyte, "/volume").catch( + (caught: unknown) => caught, + ); + + expect(error).toBeInstanceOf(InsufficientDiskSpaceError); + expect(error).toMatchObject({ + availableBytes: 4 * gibibyte, + platform: "android", + requiredBytes: 6 * gibibyte, + }); + }); +}); + +describe("DiskSpaceGuard", () => { + it("lets a single reservation through when it fits free space", async () => { + const filesystem = new MemoryFilesystem(10 * gibibyte); + const guard = new DiskSpaceGuard(); + + await expect(guard.reserve(filesystem, "ios", 6 * gibibyte, "/volume")).resolves.toBeInstanceOf( + Function, + ); + }); + + it("rejects a reservation that alone exceeds free space, with InsufficientDiskSpaceError", async () => { + const filesystem = new MemoryFilesystem(4 * gibibyte); + const guard = new DiskSpaceGuard(); + + await expect(guard.reserve(filesystem, "ios", 6 * gibibyte, "/volume")).rejects.toBeInstanceOf( + InsufficientDiskSpaceError, + ); + }); + + it("rejects a second concurrent reservation that would overfill the volume alongside the first", async () => { + const filesystem = new MemoryFilesystem(10 * gibibyte); + const guard = new DiskSpaceGuard(); + + // First reservation fits (6 of 10 GiB); still outstanding when the second is attempted. + const releaseFirst = await guard.reserve(filesystem, "ios", 6 * gibibyte, "/volume"); + + // 10 GiB free minus the 6 GiB already reserved leaves 4 GiB -- not enough for another 6 GiB, + // even though a bare disk-free reading alone would say yes. + await expect( + guard.reserve(filesystem, "android", 6 * gibibyte, "/volume"), + ).rejects.toBeInstanceOf(InsufficientDiskSpaceError); + + releaseFirst(); + }); + + it("frees the reservation on release, letting a subsequent reservation succeed", async () => { + const filesystem = new MemoryFilesystem(10 * gibibyte); + const guard = new DiskSpaceGuard(); + + const releaseFirst = await guard.reserve(filesystem, "ios", 6 * gibibyte, "/volume"); + releaseFirst(); + + await expect( + guard.reserve(filesystem, "android", 6 * gibibyte, "/volume"), + ).resolves.toBeInstanceOf(Function); + }); + + it("tracks reservations independently per path", async () => { + const filesystem = new MemoryFilesystem(10 * gibibyte); + const guard = new DiskSpaceGuard(); + + // Both reservations are 6 of the same 10 GiB free reading, but against different paths -- + // neither should see the other's outstanding bytes. + await expect( + guard.reserve(filesystem, "ios", 6 * gibibyte, "/volume-a"), + ).resolves.toBeInstanceOf(Function); + await expect( + guard.reserve(filesystem, "android", 6 * gibibyte, "/volume-b"), + ).resolves.toBeInstanceOf(Function); + }); + + it("is idempotent: releasing twice does not double-free the reservation", async () => { + const filesystem = new MemoryFilesystem(10 * gibibyte); + const guard = new DiskSpaceGuard(); + + const release = await guard.reserve(filesystem, "ios", 6 * gibibyte, "/volume"); + release(); + release(); + + // A double release must not credit the 6 GiB back twice, which would let two more 6 GiB + // reservations both succeed against only 10 GiB of real free space. + const releaseSecond = await guard.reserve(filesystem, "ios", 6 * gibibyte, "/volume"); + await expect( + guard.reserve(filesystem, "android", 6 * gibibyte, "/volume"), + ).rejects.toBeInstanceOf(InsufficientDiskSpaceError); + releaseSecond(); + }); +}); diff --git a/src/core/driver.ts b/src/core/driver.ts index 7b9d1d6..c371d22 100644 --- a/src/core/driver.ts +++ b/src/core/driver.ts @@ -101,7 +101,16 @@ export interface Driver { readonly platform: Platform; resolveSpec( request: DeviceRequest, - options: { readonly allowDownload: boolean }, + options: { + readonly allowDownload: boolean; + /** + * The requester on whose behalf this resolution runs, when known. Optional: a caller that + * resolves outside of a lease request (e.g. a driver revalidating its own cached spec) has + * no requester to attribute. Threaded through to a driver's component-install diagnostics + * so the resulting `component.install-*` events carry it -- see `docs/EVENTS.md`. + */ + readonly requesterId?: string; + }, ): Promise; provision(spec: DeviceSpec): Promise; /** @@ -205,12 +214,71 @@ export class LicenseNotAcceptedError extends Error { } } +/** + * Serializes disk-space preflight across concurrent component installs sharing a volume. + * `assertDiskSpace` alone only ever sees the disk's free space at the instant it is called: two + * installs racing the same preflight (an iOS runtime download and an Android system-image + * install, or two of either) can each observe enough free space and both proceed, jointly + * overfilling the volume neither alone would have. A single shared `DiskSpaceGuard` instance, + * injected into every driver that installs components (wired once in `src/daemon/main.ts`), + * fixes that by tracking bytes reserved but not yet released, keyed per path, and checking free + * space *minus* those outstanding reservations rather than free space alone. + * + * `reserve` resolves or throws synchronously with respect to any other in-flight `reserve` call: + * the only `await` is `filesystem.diskFree`, and the check-then-record step immediately after it + * runs to completion before any other queued continuation gets a turn (JS's single-threaded + * run-to-completion semantics), so two concurrent reservations against the same path can never + * both observe headroom the other has already claimed. + */ +export class DiskSpaceGuard { + readonly #outstandingBytesByPath = new Map(); + + /** + * Reserves `requiredBytes` against `path`'s free space, minus whatever this guard already has + * outstanding there. Throws `InsufficientDiskSpaceError` (same shape `assertDiskSpace` throws) + * when the reservation would not fit. On success, returns a release function the caller must + * invoke exactly once (typically in a `finally`) once the install this reservation was made + * for has settled, freeing the bytes for the next reservation. + */ + async reserve( + filesystem: Pick, + platform: Platform, + requiredBytes: number, + path = ".", + ): Promise<() => void> { + const availableBytes = await filesystem.diskFree(path); + const outstandingBytes = this.#outstandingBytesByPath.get(path) ?? 0; + const effectivelyAvailableBytes = availableBytes - outstandingBytes; + if (effectivelyAvailableBytes < requiredBytes) { + throw new InsufficientDiskSpaceError( + platform, + requiredBytes, + Math.max(0, effectivelyAvailableBytes), + ); + } + this.#outstandingBytesByPath.set(path, outstandingBytes + requiredBytes); + + let released = false; + return () => { + if (released) return; + released = true; + const remaining = (this.#outstandingBytesByPath.get(path) ?? 0) - requiredBytes; + if (remaining <= 0) { + this.#outstandingBytesByPath.delete(path); + } else { + this.#outstandingBytesByPath.set(path, remaining); + } + }; + } +} + /** * Checked before a driver starts any multi-GB component download/install, so a full disk fails * fast with a clear message instead of filling up mid-download (see safety rule 4's spirit -- * downloads must never surprise the machine they run on). `path` defaults to `"."`, the same * convention `CleanupReaper` uses for its own disk-pressure check (`src/core/reaper.ts`): the - * daemon process's own working-directory volume. + * daemon process's own working-directory volume. Single-shot: does not account for another + * concurrent install's own in-flight reservation -- see `DiskSpaceGuard` for that. */ export async function assertDiskSpace( filesystem: Pick, diff --git a/src/core/fake-driver.ts b/src/core/fake-driver.ts index d5afe7c..b8792e3 100644 --- a/src/core/fake-driver.ts +++ b/src/core/fake-driver.ts @@ -93,7 +93,7 @@ export class FakeDriver implements Driver { async resolveSpec( request: DeviceRequest, - options: { readonly allowDownload: boolean }, + options: { readonly allowDownload: boolean; readonly requesterId?: string }, ): Promise { await this.#beforeCall("resolveSpec", request, options); this.#assertMatchingPlatform(request.platform); diff --git a/src/core/index.ts b/src/core/index.ts index ae9367d..0870c4e 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -18,9 +18,9 @@ export { type CleanupRule, type RegistryView } from "./cleanup/types.js"; export { automaticCleanupRules } from "./cleanup/rules.js"; export { CleanupReaper } from "./reaper.js"; export { - assertDiskSpace, BootTimeoutError, type DeviceRequest, + DiskSpaceGuard, type Driver, type DriverCatalogEntry, DriverCrashError, diff --git a/src/core/lease-acquisition-coordinator.ts b/src/core/lease-acquisition-coordinator.ts index 68d2c3f..31975d2 100644 --- a/src/core/lease-acquisition-coordinator.ts +++ b/src/core/lease-acquisition-coordinator.ts @@ -211,6 +211,11 @@ export class LeaseAcquisitionCoordinator implements AcquisitionMaintenance { try { waiter.spec = await driver.resolveSpec(request, { allowDownload: options.allowDownload ?? false, + // The waiter is the one place that knows which requester triggered this resolution; + // threaded through so a component install a driver ends up doing on this request's + // behalf can attribute its diagnostics (and the resulting `component.install-*` events) + // to it. + requesterId: options.requesterId, }); } catch (error: unknown) { await this.options.decisions.run(async () => { diff --git a/src/daemon/main.test.ts b/src/daemon/main.test.ts index 3dc14ad..92c7e4b 100644 --- a/src/daemon/main.test.ts +++ b/src/daemon/main.test.ts @@ -216,6 +216,28 @@ describe("component install diagnostic bridging", () => { ]); }); + it("carries requesterId onto the bridged event when the diagnostic knows one, and omits it when it doesn't", () => { + const clock = new FakeClock(1_000); + const eventBus = new EventBus(clock); + const seen: unknown[] = []; + eventBus.subscribeAll((envelope) => seen.push(envelope)); + const bridge = emitComponentInstallDiagnostic(eventBus, "ios"); + + bridge({ componentId: "18.6", kind: "component-install-started", requesterId: "agent-1" }); + bridge({ componentId: "18.6", kind: "component-install-started" }); + + expect(seen).toEqual([ + expect.objectContaining({ + event: "component.install-started", + payload: { componentId: "18.6", platform: "ios", requesterId: "agent-1" }, + }), + expect.objectContaining({ + event: "component.install-started", + payload: { componentId: "18.6", platform: "ios" }, + }), + ]); + }); + it("forwards only component-install-* diagnostics from the Android driver's broader onDiagnostic surface", () => { const clock = new FakeClock(1_000); const eventBus = new EventBus(clock); @@ -270,6 +292,34 @@ describe("wireComponentInstallLogging", () => { ); }); + it("includes requesterId in the durable log line when the event carries one", () => { + const clock = new FakeClock(1_000); + const sink = new MemoryLogSink(); + const logger = new JsonLinesLogger({ clock, level: "debug", sink }); + const eventBus = new EventBus(clock); + + wireComponentInstallLogging(eventBus, logger); + eventBus.emit( + "component.installed", + { componentId: "18.6", durationMs: 42_000, platform: "ios", requesterId: "agent-1" }, + "driver-diagnostics", + ); + + expect(sink.records).toContainEqual( + expect.objectContaining({ + level: "info", + message: "Component installed", + module: "daemon.components", + fields: { + componentId: "18.6", + durationMs: 42_000, + platform: "ios", + requesterId: "agent-1", + }, + }), + ); + }); + it("does not log for component.install-started or component.install-failed", () => { const clock = new FakeClock(1_000); const sink = new MemoryLogSink(); diff --git a/src/daemon/main.ts b/src/daemon/main.ts index 5aa9079..1ec3a59 100644 --- a/src/daemon/main.ts +++ b/src/daemon/main.ts @@ -7,6 +7,7 @@ import { type ConfigOverrides, type Driver, CleanupReaper, + DiskSpaceGuard, Doctor, LeaseEngine, loadConfig, @@ -95,11 +96,15 @@ export async function startDaemon(options: StartDaemonOptions = {}): Promise; readonly filesystem: Filesystem; @@ -210,12 +222,14 @@ export async function discoverDrivers(options: DriverDiscoveryContext): Promise< return loadDriversModule(driversModule, options, logger); } + const diskSpaceGuard = options.diskSpaceGuard ?? new DiskSpaceGuard(); const drivers: Driver[] = []; if (process.platform === "darwin") { drivers.push( new IosSimctlDriver({ clock: options.clock, coreSimulatorRoot: `${homedir()}/Library/Developer/CoreSimulator`, + diskSpaceGuard, ...(options.downloadTimeoutMs === undefined ? {} : { downloadTimeoutMs: options.downloadTimeoutMs }), @@ -232,6 +246,7 @@ export async function discoverDrivers(options: DriverDiscoveryContext): Promise< await AndroidDriver.create({ acceptAndroidLicenses: options.acceptAndroidLicenses ?? false, clock: options.clock, + diskSpaceGuard, ...(options.downloadTimeoutMs === undefined ? {} : { downloadTimeoutMs: options.downloadTimeoutMs }), @@ -305,14 +320,27 @@ export function emitComponentInstallDiagnostic( case "component-install-started": eventBus.emit( "component.install-started", - { componentId: diagnostic.componentId, platform }, + { + componentId: diagnostic.componentId, + platform, + ...(diagnostic.requesterId === undefined + ? {} + : { requesterId: diagnostic.requesterId }), + }, "driver-diagnostics", ); return; case "component-installed": eventBus.emit( "component.installed", - { componentId: diagnostic.componentId, durationMs: diagnostic.durationMs, platform }, + { + componentId: diagnostic.componentId, + durationMs: diagnostic.durationMs, + platform, + ...(diagnostic.requesterId === undefined + ? {} + : { requesterId: diagnostic.requesterId }), + }, "driver-diagnostics", ); return; @@ -324,6 +352,9 @@ export function emitComponentInstallDiagnostic( durationMs: diagnostic.durationMs, error: diagnostic.error, platform, + ...(diagnostic.requesterId === undefined + ? {} + : { requesterId: diagnostic.requesterId }), }, "driver-diagnostics", ); @@ -375,6 +406,9 @@ export function wireComponentInstallLogging( componentId: envelope.payload.componentId, durationMs: envelope.payload.durationMs, platform: envelope.payload.platform, + ...(envelope.payload.requesterId === undefined + ? {} + : { requesterId: envelope.payload.requesterId }), }); }); } diff --git a/src/daemon/server.test.ts b/src/daemon/server.test.ts index dfc379b..51c3a30 100644 --- a/src/daemon/server.test.ts +++ b/src/daemon/server.test.ts @@ -1099,6 +1099,7 @@ describe("DaemonServer download policy", () => { expect(grant.ok).toBe(true); expect(driver.calls.find((call) => call.operation === "resolveSpec")?.arguments[1]).toEqual({ allowDownload: true, + requesterId: "agent-1", }); await client.close(); }); @@ -1122,6 +1123,7 @@ describe("DaemonServer download policy", () => { expect(response.error?.message).toContain("downloads.policy"); expect(driver.calls.find((call) => call.operation === "resolveSpec")?.arguments[1]).toEqual({ allowDownload: false, + requesterId: "agent-1", }); await client.close(); }); @@ -1148,6 +1150,28 @@ describe("DaemonServer download policy", () => { await client.close(); }); + it("attaches the download-policy suffix under the never policy even when the request itself never asked for a download", async () => { + const clock = new FakeClock(1_000); + const driver = new FakeDriver({ availableOsVersions: [], clock, platform: "ios" }); + const harness = await createHarness({ clock, downloads: { policy: "never" }, driver }); + const client = await createClient(harness.socketPath); + await hello(client); + + // No allowDownload on this request at all -- the driver's own message still suggests + // `--allow-download`, which under the never policy can never help, so the suffix must + // still attach as the correction. + const response = await client.request("lease.request", { + mode: "held", + requesterId: "agent-1", + request: { model: "iPhone 16", osVersion: "26.5", platform: "ios" }, + }); + + expect(response.ok).toBe(false); + expect(response.error).toMatchObject({ code: "RUNTIME_MISSING" }); + expect(response.error?.message).toContain("downloads.policy"); + await client.close(); + }); + it("never attaches the download-policy suffix to an undownloadable RuntimeMissingError, even under the never policy", async () => { const clock = new FakeClock(1_000); const driver = new FakeDriver({ availableOsVersions: [], clock, platform: "ios" }); diff --git a/src/daemon/server.ts b/src/daemon/server.ts index daf642d..214a77f 100644 --- a/src/daemon/server.ts +++ b/src/daemon/server.ts @@ -553,7 +553,6 @@ export class DaemonServer { // missing" message below), `always` grants it without the caller asking. const requestedAllowDownload = optionalBoolean(payload, "allowDownload") ?? false; const downloadsPolicy = this.options.config.downloads.policy; - const blockedByDownloadPolicy = downloadsPolicy === "never" && requestedAllowDownload; let progressSocket: IpcConnection | undefined = connection.socket; const disposeProgress = () => { progressSocket = undefined; @@ -576,13 +575,21 @@ export class DaemonServer { }); } catch (error: unknown) { // The driver only ever sees the clamped-to-false permission, so its own - // RuntimeMissingError just says "missing" -- it has no way to know a request asked for a - // download and config refused it. Recover that distinction here, the one place that saw - // both sides, rather than teaching the driver about config. Gated on `downloadable`: a - // request no download could ever have fixed (out of range, an installed-but-unpaired - // runtime, older than the download floor) must not be blamed on the download policy -- - // that policy was never what stood between this request and success. - if (blockedByDownloadPolicy && error instanceof RuntimeMissingError && error.downloadable) { + // RuntimeMissingError just says "missing" -- it has no way to know config is what's + // standing between this request and success. Recover that distinction here, the one place + // that saw both sides, rather than teaching the driver about config. Attach the suffix + // whenever the `never` policy is active, regardless of whether this particular request + // asked for a download: the driver's message suggests `--allow-download`, which under + // `never` can never help, so the suffix is the correction every caller needs to see, not + // just the ones that happened to ask. Still gated on `downloadable`: a request no download + // could ever have fixed (out of range, an installed-but-unpaired runtime, older than the + // download floor) must not be blamed on the download policy -- that policy was never what + // stood between this request and success. + if ( + downloadsPolicy === "never" && + error instanceof RuntimeMissingError && + error.downloadable + ) { error.message = `${error.message} (downloads are disabled by configuration: downloads.policy is "never")`; } throw error; diff --git a/src/drivers/android/device-profile-source.test.ts b/src/drivers/android/device-profile-source.test.ts index 3a25657..9f8ba02 100644 --- a/src/drivers/android/device-profile-source.test.ts +++ b/src/drivers/android/device-profile-source.test.ts @@ -96,6 +96,31 @@ describe("UserDeviceProfileSource", () => { }); }); + it("reports a devices.xml with a newline embedded in a device name as a diagnostic and produces no profile", async () => { + // Stands in for `Google\ndisk.dataPartition.path=/evil`: + // a value that would inject an arbitrary extra config.ini line once + // `AndroidDriver#applyHardwareProperties` merges it in. Embedded directly (not via an XML + // entity) since `extractText` only trims leading/trailing whitespace, not internal + // characters. + const filesystem = await filesystemWithDevicesXml( + '' + + "Evil\nPhone" + + "", + ); + const diagnostics: DeviceProfileSourceDiagnostic[] = []; + const source = new UserDeviceProfileSource(devicesXmlPath, filesystem, (diagnostic) => + diagnostics.push(diagnostic), + ); + + await expect(source.listModels()).resolves.toEqual([]); + await expect(source.resolve("Evil\nPhone")).resolves.toBeUndefined(); + expect(diagnostics).toHaveLength(2); + expect(diagnostics[0]).toMatchObject({ + kind: "device-profile-source-unreadable", + path: devicesXmlPath, + }); + }); + it("treats a well-formed but empty devices.xml as legitimately profile-less", async () => { const filesystem = await filesystemWithDevicesXml( '', @@ -154,6 +179,28 @@ describe("parseDevicesXml", () => { expect(parseDevicesXml(xml)).toEqual([]); }); + it("rejects a device name containing an embedded line break or NUL byte", () => { + const withNewline = + '' + + "Evil\nPhone"; + expect(() => parseDevicesXml(withNewline)).toThrow(); + + const withNul = + '' + + "Evil\u0000Phone"; + expect(() => parseDevicesXml(withNul)).toThrow(); + }); + + it("rejects a manufacturer value containing an embedded line break, routing config.ini injection attempts through the same rejection as the name field", () => { + const xml = + '' + + "Pixel Knockoff" + + "Google\ndisk.dataPartition.path=/evil" + + ""; + + expect(() => parseDevicesXml(xml)).toThrow(); + }); + it("returns no profiles for an empty file without throwing", () => { expect(parseDevicesXml("")).toEqual([]); expect(parseDevicesXml(" \n ")).toEqual([]); diff --git a/src/drivers/android/device-profile-source.ts b/src/drivers/android/device-profile-source.ts index 2f92811..4ae789b 100644 --- a/src/drivers/android/device-profile-source.ts +++ b/src/drivers/android/device-profile-source.ts @@ -234,6 +234,7 @@ interface DevicesXmlProfile { * from whichever `` block appears first in the file, matching `` / * `` textually rather than per-state. */ +// fallow-ignore-next-line complexity -- per-field extraction and rejection checks are one parse pass over one block. export function parseDevicesXml(contents: string): readonly DevicesXmlProfile[] { const trimmed = contents.trim(); if (trimmed === "") { @@ -249,22 +250,54 @@ export function parseDevicesXml(contents: string): readonly DevicesXmlProfile[] if (rawName === undefined || rawName === "") { continue; } + // A value containing CR, LF, or NUL is never a legitimate device name or property -- + // devices.xml is Android Studio's own file, but Simlock only ever reads it (safety rule 1), + // and every value here eventually flows into a `config.ini` line-merge + // (`AndroidDriver#applyHardwareProperties` -> `#mergeConfigIniLines`). A newline there would + // inject an arbitrary extra `config.ini` key. Thrown rather than silently skipped or + // sanitized: this routes through the same malformed-devices.xml diagnostic path the caller + // (`UserDeviceProfileSource#profiles`) already has for an unparseable file, so a poisoned + // value is surfaced rather than quietly dropped. + if (containsForbiddenCharacter(rawName)) { + throw new Error(`devices.xml device name contains an embedded line break or NUL byte`); + } const name = unescapeXml(rawName); const hardwareProperties: Record = { "hw.device.name": name }; const manufacturer = extractText(block, "manufacturer"); if (manufacturer !== undefined && manufacturer !== "") { + if (containsForbiddenCharacter(manufacturer)) { + throw new Error( + `devices.xml manufacturer for device "${name}" contains an embedded line break or NUL byte`, + ); + } hardwareProperties["hw.device.manufacturer"] = unescapeXml(manufacturer); } applyScreenProperties(block, hardwareProperties); applyRamProperty(block, hardwareProperties); + // Defense in depth beyond the per-field checks above: every value about to leave this + // parser (including ones a future field addition might forget to check individually) must + // be a single line before it is handed to the driver -- the same invariant + // `AndroidDriver#mergeConfigIniLines` enforces again on its own side, independently. + if (Object.values(hardwareProperties).some(containsForbiddenCharacter)) { + throw new Error( + `devices.xml property for device "${name}" contains an embedded line break or NUL byte`, + ); + } + profiles.push({ hardwareProperties, name }); } return profiles; } +/** CR, LF, or NUL -- see the rejection check at the top of the `` loop above. */ +function containsForbiddenCharacter(value: string): boolean { + // oxlint-disable-next-line no-control-regex -- NUL rejection is intentional, not an accidental control-character match. + return /[\r\n\u0000]/.test(value); +} + function applyScreenProperties( deviceBlock: string, hardwareProperties: Record, diff --git a/src/drivers/android/index.test.ts b/src/drivers/android/index.test.ts index 36fefdf..5381ae2 100644 --- a/src/drivers/android/index.test.ts +++ b/src/drivers/android/index.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it, vi } from "vitest"; import type { Driver } from "../../core/driver.js"; -import { InsufficientDiskSpaceError } from "../../core/index.js"; +import { DiskSpaceGuard, InsufficientDiskSpaceError } from "../../core/index.js"; import { FakeClock, type Filesystem, @@ -9,6 +9,8 @@ import { NodeFilesystem, NodeProcessRunner, ScriptedProcessRunner, + type ProcessHandle, + type ProcessRunOptions, type ScriptedProcessExpectation, SystemClock, } from "../../ports/index.js"; @@ -101,14 +103,17 @@ describe("AndroidDriver", () => { it("fails for a missing image unless downloads are explicitly allowed", async () => { const filesystem = await androidFilesystem(); - const runner = new ScriptedProcessRunner([ - processResult(binaries.avdmanager, ["list", "device"], pixelDevices), - processResult(binaries.avdmanager, ["list", "device"], pixelDevices), - processResult(binaries.sdkmanager, [ - "--install", - "system-images;android-35;google_apis;arm64-v8a", - ]), - ]); + const runner = new InstallReflectingProcessRunner( + [ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.sdkmanager, [ + "--install", + "system-images;android-35;google_apis;arm64-v8a", + ]), + ], + filesystem, + ); const driver = await createDriver(filesystem, runner); const request = { model: "Pixel 8", osVersion: "35", platform: "android" } as const; @@ -993,6 +998,69 @@ describe("AndroidDriver", () => { ); }); + it("rejects a hardware-property value with an embedded line break, defense in depth beyond the devices.xml parser", async () => { + // `UserDeviceProfileSource`/`parseDevicesXml` already reject this at the devices.xml parse + // boundary (see device-profile-source.test.ts), but `DeviceProfileSource` is a documented + // extension point (see the interface's own doc comment) -- a future or third-party source + // could hand the driver a multiline value directly, bypassing that parser entirely. This + // exercises `#mergeConfigIniLines`'s own independent guard by going around the parser with + // a custom source, standing in for `Google\ndisk.dataPartition.path=/evil + // `. + const filesystem = await androidFilesystem(); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.avdmanager, [ + "create", + "avd", + "-n", + "simlock_one", + "-k", + /.+/, + "-d", + "pixel_8", + ]), + ]); + const maliciousSource = { + async listModels() { + return ["Evil Phone"]; + }, + async resolve(model: string) { + if (model.toLocaleLowerCase() !== "evil phone") { + return undefined; + } + return { + hardwareProperties: { + "hw.device.manufacturer": "Acme\ndisk.dataPartition.path=/evil", + "hw.device.name": "Evil Phone", + }, + kind: "properties" as const, + name: "Evil Phone", + }; + }, + }; + const driver = await AndroidDriver.create({ + clock: new FakeClock(), + deviceProfileSources: [maliciousSource], + env: { ANDROID_HOME: sdk }, + filesystem, + homeDirectory: home, + hostAbi: "arm64-v8a", + idGenerator: { generate: () => "one" }, + processRunner: runner, + }); + + const spec = await driver.resolveSpec( + { model: "Evil Phone", osVersion: "34", platform: "android" }, + { allowDownload: false }, + ); + + await expect(driver.provision(spec)).rejects.toThrow(/line break/); + // The rejected merge must never have reached the filesystem at all. + await expect(filesystem.exists(`${avdDirectory}/simlock_one.avd/config.ini`)).resolves.toBe( + false, + ); + }); + it("surfaces malformed devices.xml as a diagnostic and falls through to UnknownModelError, never throwing from the parse itself", async () => { const filesystem = await androidFilesystem(); await filesystem.mkdirp(`${home}/.android`); @@ -1050,7 +1118,14 @@ describe("AndroidDriver", () => { expect((error as Error).message).toContain("sdkmanager --licenses"); }); - it("accepts licenses through piped confirmation and retries the install once when the flag is on", async () => { + it("recognizes the alternate 'licenses have not been accepted' sdkmanager phrasing, not just 'not accepted'", async () => { + // The two documented sdkmanager phrasings this driver's license detection claims to + // handle (see the comment on `hasUnacceptedLicense`): a per-package warning ("... not + // accepted.") and this one, an aggregate summary with "been" between "not" and "accepted". + const licensesHaveNotBeenAcceptedOutput = + "1 of 7 SDK package license(s) not accepted.\n" + + "Review licenses that have not been accepted (see above)\n" + + "The licenses have not been accepted.\n"; const filesystem = await androidFilesystem(); const runner = new ScriptedProcessRunner([ processResult(binaries.avdmanager, ["list", "device"], pixelDevices), @@ -1059,14 +1134,41 @@ describe("AndroidDriver", () => { args: ["--install", "system-images;android-35;google_apis;arm64-v8a"], command: binaries.sdkmanager, }, - result: { code: 1, stderr: "", stdout: licenseNotAcceptedOutput }, + result: { code: 1, stderr: "", stdout: licensesHaveNotBeenAcceptedOutput }, }, - processResult(binaries.sdkmanager, ["--licenses"], "All licenses accepted.\n"), - processResult(binaries.sdkmanager, [ - "--install", - "system-images;android-35;google_apis;arm64-v8a", - ]), ]); + const driver = await createDriver(filesystem, runner, { acceptAndroidLicenses: false }); + + const error = await driver + .resolveSpec( + { model: "Pixel 8", osVersion: "35", platform: "android" }, + { allowDownload: true }, + ) + .catch((caught: unknown) => caught); + + expect(error).toBeInstanceOf(AndroidLicenseNotAcceptedError); + }); + + it("accepts licenses through piped confirmation and retries the install once when the flag is on", async () => { + const filesystem = await androidFilesystem(); + const runner = new InstallReflectingProcessRunner( + [ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + { + match: { + args: ["--install", "system-images;android-35;google_apis;arm64-v8a"], + command: binaries.sdkmanager, + }, + result: { code: 1, stderr: "", stdout: licenseNotAcceptedOutput }, + }, + processResult(binaries.sdkmanager, ["--licenses"], "All licenses accepted.\n"), + processResult(binaries.sdkmanager, [ + "--install", + "system-images;android-35;google_apis;arm64-v8a", + ]), + ], + filesystem, + ); const driver = await createDriver(filesystem, runner, { acceptAndroidLicenses: true }); await expect( @@ -1117,14 +1219,17 @@ describe("AndroidDriver", () => { it("dedupes concurrent resolveSpec calls for the same missing system image behind one sdkmanager install", async () => { const filesystem = await androidFilesystem(); - const runner = new ScriptedProcessRunner([ - processResult(binaries.avdmanager, ["list", "device"], pixelDevices), - processResult(binaries.avdmanager, ["list", "device"], pixelDevices), - processResult(binaries.sdkmanager, [ - "--install", - "system-images;android-35;google_apis;arm64-v8a", - ]), - ]); + const runner = new InstallReflectingProcessRunner( + [ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.sdkmanager, [ + "--install", + "system-images;android-35;google_apis;arm64-v8a", + ]), + ], + filesystem, + ); const driver = await createDriver(filesystem, runner); const request = { model: "Pixel 8", osVersion: "35", platform: "android" } as const; @@ -1141,13 +1246,16 @@ describe("AndroidDriver", () => { describe("component install diagnostics", () => { it("reports component-install-started then component-installed with a duration on a clean install", async () => { const filesystem = await androidFilesystem(); - const runner = new ScriptedProcessRunner([ - processResult(binaries.avdmanager, ["list", "device"], pixelDevices), - processResult(binaries.sdkmanager, [ - "--install", - "system-images;android-35;google_apis;arm64-v8a", - ]), - ]); + const runner = new InstallReflectingProcessRunner( + [ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.sdkmanager, [ + "--install", + "system-images;android-35;google_apis;arm64-v8a", + ]), + ], + filesystem, + ); const diagnostics: AndroidDriverDiagnostic[] = []; const driver = await createDriver(filesystem, runner, { onDiagnostic: (diagnostic) => diagnostics.push(diagnostic), @@ -1298,13 +1406,16 @@ describe("AndroidDriver", () => { } const recordingFilesystem = new RecordingFilesystem(); const filesystem = await androidFilesystem({}, recordingFilesystem); - const runner = new ScriptedProcessRunner([ - processResult(binaries.avdmanager, ["list", "device"], pixelDevices), - processResult(binaries.sdkmanager, [ - "--install", - "system-images;android-35;google_apis;arm64-v8a", - ]), - ]); + const runner = new InstallReflectingProcessRunner( + [ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.sdkmanager, [ + "--install", + "system-images;android-35;google_apis;arm64-v8a", + ]), + ], + filesystem, + ); const driver = await createDriver(filesystem, runner); await driver.resolveSpec( @@ -1314,11 +1425,12 @@ describe("AndroidDriver", () => { expect(recordingFilesystem.diskFreePaths).toEqual([sdk]); }); - }); - describe("download timeout", () => { - it("threads the configured downloadTimeoutMs into the sdkmanager install call", async () => { + it("reports component-install-failed, never component-installed, when sdkmanager exits 0 but the image never shows up", async () => { const filesystem = await androidFilesystem(); + // Deliberately a plain ScriptedProcessRunner, not InstallReflectingProcessRunner: sdkmanager + // claims success, but nothing ever lands in the filesystem's system-images tree -- the + // "reported success but still not installed" case the post-install re-scan exists to catch. const runner = new ScriptedProcessRunner([ processResult(binaries.avdmanager, ["list", "device"], pixelDevices), processResult(binaries.sdkmanager, [ @@ -1326,6 +1438,111 @@ describe("AndroidDriver", () => { "system-images;android-35;google_apis;arm64-v8a", ]), ]); + const diagnostics: AndroidDriverDiagnostic[] = []; + const driver = await createDriver(filesystem, runner, { + onDiagnostic: (diagnostic) => diagnostics.push(diagnostic), + }); + + const error = await driver + .resolveSpec( + { model: "Pixel 8", osVersion: "35", platform: "android" }, + { allowDownload: true }, + ) + .catch((caught: unknown) => caught); + + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toContain( + "sdkmanager reported success but system-images;android-35;google_apis;arm64-v8a is still not installed", + ); + expect(diagnostics).toEqual([ + { + componentId: "system-images;android-35;google_apis;arm64-v8a", + kind: "component-install-started", + }, + { + componentId: "system-images;android-35;google_apis;arm64-v8a", + durationMs: 0, + error: expect.stringContaining("still not installed"), + kind: "component-install-failed", + }, + ]); + }); + + it("carries requesterId through to component-install diagnostics when resolveSpec's caller knows one", async () => { + const filesystem = await androidFilesystem(); + const runner = new InstallReflectingProcessRunner( + [ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.sdkmanager, [ + "--install", + "system-images;android-35;google_apis;arm64-v8a", + ]), + ], + filesystem, + ); + const diagnostics: AndroidDriverDiagnostic[] = []; + const driver = await createDriver(filesystem, runner, { + onDiagnostic: (diagnostic) => diagnostics.push(diagnostic), + }); + + await driver.resolveSpec( + { model: "Pixel 8", osVersion: "35", platform: "android" }, + { allowDownload: true, requesterId: "agent-7" }, + ); + + expect(diagnostics).toEqual([ + { + componentId: "system-images;android-35;google_apis;arm64-v8a", + kind: "component-install-started", + requesterId: "agent-7", + }, + { + componentId: "system-images;android-35;google_apis;arm64-v8a", + durationMs: 0, + kind: "component-installed", + requesterId: "agent-7", + }, + ]); + }); + + it("respects disk-space reservations already outstanding on a shared DiskSpaceGuard", async () => { + const filesystem = await androidFilesystem({ freeDiskBytes: 2.5 * 1024 ** 3 }); + const runner = new ScriptedProcessRunner([ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + ]); + const diskSpaceGuard = new DiskSpaceGuard(); + // Stands in for another driver's (or another install's) concurrent reservation against the + // same shared guard -- 2 of the 2.5 GiB free is already spoken for, leaving less than the + // 2 GiB `ANDROID_SYSTEM_IMAGE_MIN_FREE_BYTES` floor this install needs. + const releaseOther = await diskSpaceGuard.reserve(filesystem, "ios", 1.5 * 1024 ** 3, sdk); + const driver = await createDriver(filesystem, runner, { diskSpaceGuard }); + + const error = await driver + .resolveSpec( + { model: "Pixel 8", osVersion: "35", platform: "android" }, + { allowDownload: true }, + ) + .catch((caught: unknown) => caught); + + expect(error).toBeInstanceOf(InsufficientDiskSpaceError); + expect(runner.calls.some((call) => call.command === binaries.sdkmanager)).toBe(false); + releaseOther(); + }); + }); + + describe("download timeout", () => { + it("threads the configured downloadTimeoutMs into the sdkmanager install call", async () => { + const filesystem = await androidFilesystem(); + const runner = new InstallReflectingProcessRunner( + [ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.sdkmanager, [ + "--install", + "system-images;android-35;google_apis;arm64-v8a", + ]), + ], + filesystem, + ); const driver = await createDriver(filesystem, runner, { downloadTimeoutMs: 42_000 }); await driver.resolveSpec( @@ -1341,13 +1558,16 @@ describe("AndroidDriver", () => { it("defaults to the same 20-minute timeout as before this option existed", async () => { const filesystem = await androidFilesystem(); - const runner = new ScriptedProcessRunner([ - processResult(binaries.avdmanager, ["list", "device"], pixelDevices), - processResult(binaries.sdkmanager, [ - "--install", - "system-images;android-35;google_apis;arm64-v8a", - ]), - ]); + const runner = new InstallReflectingProcessRunner( + [ + processResult(binaries.avdmanager, ["list", "device"], pixelDevices), + processResult(binaries.sdkmanager, [ + "--install", + "system-images;android-35;google_apis;arm64-v8a", + ]), + ], + filesystem, + ); const driver = await createDriver(filesystem, runner); await driver.resolveSpec( @@ -1508,6 +1728,7 @@ async function createDriver( options: { readonly acceptAndroidLicenses?: boolean; readonly clock?: FakeClock; + readonly diskSpaceGuard?: DiskSpaceGuard; readonly downloadTimeoutMs?: number; readonly ids?: readonly string[]; readonly onDiagnostic?: (diagnostic: AndroidDriverDiagnostic) => void; @@ -1520,6 +1741,7 @@ async function createDriver( ? {} : { acceptAndroidLicenses: options.acceptAndroidLicenses }), clock: options.clock ?? new FakeClock(), + ...(options.diskSpaceGuard === undefined ? {} : { diskSpaceGuard: options.diskSpaceGuard }), ...(options.downloadTimeoutMs === undefined ? {} : { downloadTimeoutMs: options.downloadTimeoutMs }), @@ -1686,6 +1908,49 @@ function baselineBuildExpectations(options: { return expectations; } +/** + * A `ScriptedProcessRunner` that also mirrors what real `sdkmanager` does on disk: a + * successful (`code: 0`, no unaccepted-license text) `--install ` call creates the + * corresponding `system-images/android-//` directory in `filesystem`. The + * driver's post-install `#installSystemImageOnce` re-scan needs the filesystem to actually + * reflect the "install" the same way the iOS driver's fixtures script a second `simctl list` + * response after a download (see `listFixtureAfterDownload` in the iOS driver's test file) -- + * a bare `ScriptedProcessRunner` only scripts the process's own stdout/stderr/exit code, never + * a filesystem side effect, so a scripted mkdirp-free "success" would fail the re-scan. + */ +class InstallReflectingProcessRunner extends ScriptedProcessRunner { + readonly #filesystem: MemoryFilesystem; + + constructor(expectations: readonly ScriptedProcessExpectation[], filesystem: MemoryFilesystem) { + super(expectations); + this.#filesystem = filesystem; + } + + override spawn( + command: string, + args: readonly string[], + options: ProcessRunOptions = {}, + ): ProcessHandle { + const handle = super.spawn(command, args, options); + if (args[0] === "--install" && typeof args[1] === "string") { + const packageName = args[1]; + void handle.wait().then((result) => { + const combined = `${result.stdout}\n${result.stderr}`; + const licenseNotAccepted = + /licen[cs]e/i.test(combined) && /not (?:been )?accepted/i.test(combined); + if (result.code === 0 && !licenseNotAccepted) { + const match = /^system-images;android-(.+);(.+);(.+)$/.exec(packageName); + if (match !== null) { + const [, api, tag, abi] = match; + void this.#filesystem.mkdirp(`${sdk}/system-images/android-${api}/${tag}/${abi}`); + } + } + }); + } + return handle; + } +} + function processResult(command: string, args: readonly (string | RegExp)[], stdout = "") { return { match: { args, command }, diff --git a/src/drivers/android/index.ts b/src/drivers/android/index.ts index 7ec7516..c4b13ee 100644 --- a/src/drivers/android/index.ts +++ b/src/drivers/android/index.ts @@ -1,8 +1,8 @@ import type { DeviceSpec } from "../../core/domain.js"; import { - assertDiskSpace, BootTimeoutError, type DeviceRequest, + DiskSpaceGuard, type Driver, type DriverCatalogEntry, type DriverDevice, @@ -95,6 +95,14 @@ export interface AndroidDriverOptions { * `~/.android/devices.xml`, so a name defined in both resolves to the built-in. */ readonly deviceProfileSources?: readonly DeviceProfileSource[]; + /** + * Disk-space preflight, shared with every other driver that installs components -- see the + * iOS driver's `IosSimctlDriverOptions.diskSpaceGuard` for why a bare `assertDiskSpace` call + * isn't enough on its own. Defaults to a private, driver-local guard when omitted (tests, + * `SIMLOCK_DRIVERS_MODULE`); production wiring (`src/daemon/main.ts`) passes one shared + * instance to every driver. + */ + readonly diskSpaceGuard?: DiskSpaceGuard; /** Per-install timeout for `sdkmanager`; defaults to `downloads.timeoutMs`'s own default. */ readonly downloadTimeoutMs?: number; readonly env: Readonly>; @@ -162,6 +170,7 @@ export class AndroidDriver implements Driver { readonly #clock: Clock; readonly #deviceProfiles: DeviceProfileRegistry; readonly #devices = new Map(); + readonly #diskSpaceGuard: DiskSpaceGuard; readonly #downloadTimeoutMs: number; readonly #filesystem: Filesystem; readonly #hostAbi: string; @@ -179,6 +188,7 @@ export class AndroidDriver implements Driver { private constructor(options: AndroidDriverOptions, sdk: AndroidSdkPaths) { this.#acceptAndroidLicenses = options.acceptAndroidLicenses ?? false; this.#clock = options.clock; + this.#diskSpaceGuard = options.diskSpaceGuard ?? new DiskSpaceGuard(); this.#downloadTimeoutMs = options.downloadTimeoutMs ?? DEFAULT_DOWNLOAD_TIMEOUT_MS; this.#filesystem = options.filesystem; this.#hostAbi = options.hostAbi ?? hostAbiFor(process.arch); @@ -205,7 +215,7 @@ export class AndroidDriver implements Driver { async resolveSpec( request: DeviceRequest, - options: { readonly allowDownload: boolean }, + options: { readonly allowDownload: boolean; readonly requesterId?: string }, ): Promise { if (request.platform !== this.platform) { throw new Error(`Android driver cannot resolve ${request.platform} requests`); @@ -224,7 +234,7 @@ export class AndroidDriver implements Driver { } const packageName = systemImagePackage(apiLevel, "google_apis", this.#hostAbi); - await this.#installSystemImage(packageName); + await this.#installSystemImage(packageName, options.requesterId); } this.#resolvedProfiles.set(profile.name.toLocaleLowerCase(), profile); @@ -606,13 +616,13 @@ export class AndroidDriver implements Driver { * failure), so a later, non-concurrent call starts a fresh attempt rather than replaying a * stale result. */ - async #installSystemImage(packageName: string): Promise { + async #installSystemImage(packageName: string, requesterId: string | undefined): Promise { const inFlight = this.#installLocks.get(packageName); if (inFlight !== undefined) { return inFlight; } - const promise = this.#installSystemImageOnce(packageName).finally(() => { + const promise = this.#installSystemImageOnce(packageName, requesterId).finally(() => { if (this.#installLocks.get(packageName) === promise) { this.#installLocks.delete(packageName); } @@ -622,39 +632,78 @@ export class AndroidDriver implements Driver { } /** - * Disk preflight, then the actual `sdkmanager` install, wrapped with `component.install-*` - * diagnostics -- split from `#installSystemImageOrThrow` below so the license-retry branching - * stays its own single-responsibility function rather than growing this one's complexity. A - * preflight failure is reported before any diagnostic fires: no install was actually - * attempted, so there is nothing to report as started or failed. The try/catch means a caller - * sees exactly one `install-failed` regardless of which branch below throws, never one per - * attempt. + * Disk preflight (via the shared `DiskSpaceGuard`, released once the install settles either + * way), then the actual `sdkmanager` install, wrapped with `component.install-*` diagnostics + * -- split from `#installSystemImageOrThrow` below so the license-retry branching stays its + * own single-responsibility function rather than growing this one's complexity. A preflight + * failure is reported before any diagnostic fires: no install was actually attempted, so + * there is nothing to report as started or failed. The try/catch means a caller sees exactly + * one `install-failed` regardless of which branch below throws, never one per attempt. + * + * `component-installed` is a verified fact, not "`sdkmanager` exited 0 (possibly after a + * license-accept retry)": once the install call itself succeeds, this re-scans + * `#installedImages` and only reports `component-installed` once the package actually + * installed is present there. Absent (a "reported success but nothing showed up" case) + * reports `component-install-failed` instead and throws, matching the iOS driver's + * post-download verification. */ - async #installSystemImageOnce(packageName: string): Promise { - await assertDiskSpace( + // fallow-ignore-next-line complexity -- reservation, install, and post-install verification are one attempt with one exit per outcome. + async #installSystemImageOnce( + packageName: string, + requesterId: string | undefined, + ): Promise { + const release = await this.#diskSpaceGuard.reserve( this.#filesystem, this.platform, ANDROID_SYSTEM_IMAGE_MIN_FREE_BYTES, this.#sdk.root, ); - this.#onDiagnostic?.({ componentId: packageName, kind: "component-install-started" }); - const startedAt = this.#clock.now(); try { - await this.#installSystemImageOrThrow(packageName); - } catch (error: unknown) { + this.#onDiagnostic?.({ + componentId: packageName, + kind: "component-install-started", + ...(requesterId === undefined ? {} : { requesterId }), + }); + const startedAt = this.#clock.now(); + try { + await this.#installSystemImageOrThrow(packageName); + } catch (error: unknown) { + this.#onDiagnostic?.({ + componentId: packageName, + durationMs: this.#clock.now() - startedAt, + error: stableError(error), + kind: "component-install-failed", + ...(requesterId === undefined ? {} : { requesterId }), + }); + throw error; + } + + const images = await this.#installedImages(); + if ( + !images.some( + (image) => systemImagePackage(image.apiLevel, image.tag, image.abi) === packageName, + ) + ) { + const message = `sdkmanager reported success but ${packageName} is still not installed`; + this.#onDiagnostic?.({ + componentId: packageName, + durationMs: this.#clock.now() - startedAt, + error: message, + kind: "component-install-failed", + ...(requesterId === undefined ? {} : { requesterId }), + }); + throw new DriverCrashError(message); + } + this.#onDiagnostic?.({ componentId: packageName, durationMs: this.#clock.now() - startedAt, - error: stableError(error), - kind: "component-install-failed", + kind: "component-installed", + ...(requesterId === undefined ? {} : { requesterId }), }); - throw error; + } finally { + release(); } - this.#onDiagnostic?.({ - componentId: packageName, - durationMs: this.#clock.now() - startedAt, - kind: "component-installed", - }); } /** @@ -818,11 +867,26 @@ export class AndroidDriver implements Driver { * merge from empty content; any other read failure is rethrown rather than treated as an * empty file -- silently starting from "" on, say, an EACCES or EIO would write back only * `entries` and clobber whatever config.ini already held. + * + * Defense in depth against a config.ini injection: `#applyHardwareProperties` calls this with + * values sourced from a device profile (`avdmanager list device`, or a parsed + * `~/.android/devices.xml` -- see `device-profile-source.ts`'s own line-break rejection at the + * parse boundary). A key or value containing a line break would let one logical property + * inject arbitrary extra `config.ini` lines once joined in -- rejected here unconditionally, + * independent of and in addition to that parse-time check, so this merge is never the only + * thing standing between untrusted input and config.ini. */ async #mergeConfigIniLines( avdName: string, entries: Readonly>, ): Promise { + for (const [key, value] of Object.entries(entries)) { + if (containsLineBreak(key) || containsLineBreak(value)) { + throw new DriverCrashError( + `Refusing to merge config.ini entry with an embedded line break (key ${JSON.stringify(key)})`, + ); + } + } const path = this.#configIniPath(avdName); let contents: string; try { @@ -1331,6 +1395,11 @@ function portsFromAdbDevices(output: string): number[] { .filter((port) => Number.isInteger(port)); } +/** See `#mergeConfigIniLines`'s defense-in-depth check. */ +function containsLineBreak(value: string): boolean { + return /[\r\n]/.test(value); +} + function stableHash(parts: readonly string[]): string { let hash = 0x811c9dc5; for (const character of parts.join("\u0000")) { @@ -1353,7 +1422,9 @@ function hostAbiFor(architecture: string): string { */ function hasUnacceptedLicense(result: ProcessResult): boolean { const combined = `${result.stdout}\n${result.stderr}`; - return /licen[cs]e/i.test(combined) && /not accepted/i.test(combined); + // Covers both documented sdkmanager phrasings: "License for package ... not accepted." and + // "licenses have not been accepted." -- the latter has "been" between "not" and "accepted". + return /licen[cs]e/i.test(combined) && /not (?:been )?accepted/i.test(combined); } /** diff --git a/src/drivers/diagnostics.ts b/src/drivers/diagnostics.ts index 159470b..4672cf9 100644 --- a/src/drivers/diagnostics.ts +++ b/src/drivers/diagnostics.ts @@ -10,15 +10,22 @@ * -- see `emitComponentInstallDiagnostic` in `src/daemon/main.ts`. */ export type ComponentInstallDiagnostic = - | { readonly kind: "component-install-started"; readonly componentId: string } + | { + readonly kind: "component-install-started"; + readonly componentId: string; + /** The requester on whose behalf this install runs, when the resolution that triggered it knew one. */ + readonly requesterId?: string; + } | { readonly kind: "component-installed"; readonly componentId: string; readonly durationMs: number; + readonly requesterId?: string; } | { readonly kind: "component-install-failed"; readonly componentId: string; readonly durationMs: number; readonly error: string; + readonly requesterId?: string; }; diff --git a/src/drivers/ios/index.test.ts b/src/drivers/ios/index.test.ts index d3a9b1b..52588f5 100644 --- a/src/drivers/ios/index.test.ts +++ b/src/drivers/ios/index.test.ts @@ -4,6 +4,7 @@ import { describe, expect, it } from "vitest"; import { BootTimeoutError, + DiskSpaceGuard, DriverCrashError, InsufficientDiskSpaceError, RuntimeMissingError, @@ -301,6 +302,110 @@ describe("IosSimctlDriver", () => { expect(runner.calls.filter((call) => call.command === "xcodebuild")).toHaveLength(1); }); + it("rejects a freshly downloaded exact-version runtime that does not pair with the requested device type", async () => { + // Mirrors the already-installed pairing check (`rejects an installed runtime whose + // supportedDeviceTypes omits the requested model` above), but for a runtime that only shows + // up *after* the download -- the refreshed catalog's iOS 18.6 exists but pairs with nothing. + const unpairedAfterDownload = JSON.stringify({ + devicetypes: (JSON.parse(listFixture) as { devicetypes: unknown }).devicetypes, + runtimes: [ + ...(JSON.parse(listFixture) as { runtimes: unknown[] }).runtimes, + { + identifier: "com.apple.CoreSimulator.SimRuntime.iOS-18-6", + isAvailable: true, + name: "iOS 18.6", + supportedDeviceTypes: [], + version: "18.6", + }, + ], + }); + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixture } }, + { + match: { + command: "xcodebuild", + args: ["-downloadPlatform", "iOS", "-buildVersion", "18.6"], + }, + }, + { match: listInvocation, result: { code: 0, stderr: "", stdout: unpairedAfterDownload } }, + ]); + const driver = createDriver(runner); + + const result = await driver + .resolveSpec( + { model: "iPhone 16", osVersion: "18.6", platform: "ios" }, + { allowDownload: true }, + ) + .catch((error: unknown) => error); + + expect(result).toBeInstanceOf(RuntimeMissingError); + expect(result).toMatchObject({ + downloadable: false, + message: expect.stringContaining("does not support iPhone 16"), + }); + // Downloaded, but never committed to a spec: no simctl create followed the failed pairing check. + expect(runner.calls.map((call) => call.command)).toEqual(["xcrun", "xcodebuild", "xcrun"]); + }); + + describe("empty runtime catalog", () => { + // Devicetypes come from the Xcode install itself and are never empty on a working + // toolchain (an empty list there still means malformed JSON), but a fresh Xcode with zero + // simulator runtimes installed is a normal starting state -- `parseCatalog` must let it + // through so `resolveSpec` can reach the download-latest path instead of failing before any + // resolution is attempted. + const emptyRuntimesCatalog = JSON.stringify({ + devicetypes: [ + { + identifier: "com.apple.CoreSimulator.SimDeviceType.iPhone-16", + maxRuntimeVersion: 16_777_215, + minRuntimeVersion: 0, + name: "iPhone 16", + }, + ], + runtimes: [], + }); + + it("resolves via the download path when allowDownload is true", async () => { + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: emptyRuntimesCatalog } }, + { match: { command: "xcodebuild", args: ["-downloadPlatform", "iOS"] } }, + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixture } }, + ]); + const driver = createDriver(runner); + + await expect( + driver.resolveSpec({ model: "iPhone 16", platform: "ios" }, { allowDownload: true }), + ).resolves.toEqual({ model: "iPhone 16", osVersion: "26.5", platform: "ios" }); + }); + + it("gives a clean RuntimeMissingError, not a parse-time DriverCrashError, when allowDownload is false", async () => { + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: emptyRuntimesCatalog } }, + ]); + const driver = createDriver(runner); + + const result = await driver + .resolveSpec({ model: "iPhone 16", platform: "ios" }, { allowDownload: false }) + .catch((error: unknown) => error); + + expect(result).toBeInstanceOf(RuntimeMissingError); + expect(result).not.toBeInstanceOf(DriverCrashError); + }); + + it("lists an empty runtimes catalog with defaultRuntime undefined instead of throwing", async () => { + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: emptyRuntimesCatalog } }, + ]); + const driver = createDriver(runner); + + await expect(driver.listCatalog()).resolves.toEqual({ + defaultRuntime: undefined, + models: ["iPhone 16"], + runtimes: [], + }); + }); + }); + describe("component install diagnostics", () => { it("reports component-install-started then component-installed with a duration on a successful download", async () => { const runner = new ScriptedProcessRunner([ @@ -509,6 +614,104 @@ describe("IosSimctlDriver", () => { expect(filesystem.diskFreePaths).toEqual(["/Users/agent/Library/Developer/CoreSimulator"]); }); + + it("reports component-install-failed, never component-installed, when xcodebuild exits 0 but the runtime never shows up", async () => { + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixture } }, + { + match: { + command: "xcodebuild", + args: ["-downloadPlatform", "iOS", "-buildVersion", "18.6"], + }, + }, + // Deliberately re-scans to the SAME catalog: xcodebuild claims success, but no iOS 18.6 + // runtime is present -- the "reported success but still not installed" case the + // post-download re-scan exists to catch. + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixture } }, + ]); + const diagnostics: ComponentInstallDiagnostic[] = []; + const driver = createDriver(runner, new FakeClock(), new MemoryFilesystem(), (diagnostic) => + diagnostics.push(diagnostic), + ); + + const error = await driver + .resolveSpec( + { model: "iPhone 16", osVersion: "18.6", platform: "ios" }, + { allowDownload: true }, + ) + .catch((caught: unknown) => caught); + + expect(error).toBeInstanceOf(DriverCrashError); + expect((error as Error).message).toContain("iOS 18.6 is still not installed"); + expect(diagnostics).toEqual([ + { componentId: "18.6", kind: "component-install-started" }, + { + componentId: "18.6", + durationMs: 0, + error: expect.stringContaining("still not installed"), + kind: "component-install-failed", + }, + ]); + }); + + it("carries requesterId through to component-install diagnostics when resolveSpec's caller knows one", async () => { + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixture } }, + { + match: { + command: "xcodebuild", + args: ["-downloadPlatform", "iOS", "-buildVersion", "18.6"], + }, + }, + { + match: listInvocation, + result: { code: 0, stderr: "", stdout: listFixtureAfterDownload }, + }, + ]); + const diagnostics: ComponentInstallDiagnostic[] = []; + const driver = createDriver(runner, new FakeClock(), new MemoryFilesystem(), (diagnostic) => + diagnostics.push(diagnostic), + ); + + await driver.resolveSpec( + { model: "iPhone 16", osVersion: "18.6", platform: "ios" }, + { allowDownload: true, requesterId: "agent-7" }, + ); + + expect(diagnostics).toEqual([ + { componentId: "18.6", kind: "component-install-started", requesterId: "agent-7" }, + { + componentId: "18.6", + durationMs: 0, + kind: "component-installed", + requesterId: "agent-7", + }, + ]); + }); + + it("respects disk-space reservations already outstanding on a shared DiskSpaceGuard", async () => { + const runner = new ScriptedProcessRunner([ + { match: listInvocation, result: { code: 0, stderr: "", stdout: listFixture } }, + ]); + const filesystem = new MemoryFilesystem(9 * 1024 ** 3); + const diskSpaceGuard = new DiskSpaceGuard(); + // Stands in for another driver's (or another install's) concurrent reservation against the + // same shared guard -- 2 of the 9 GiB free is already spoken for, leaving less than the + // 8 GiB `IOS_RUNTIME_MIN_FREE_BYTES` floor this download needs. + const releaseOther = await diskSpaceGuard.reserve(filesystem, "android", 2 * 1024 ** 3, "."); + const driver = createDriver(runner, new FakeClock(), filesystem, undefined, diskSpaceGuard); + + const error = await driver + .resolveSpec( + { model: "iPhone 16", osVersion: "18.6", platform: "ios" }, + { allowDownload: true }, + ) + .catch((caught: unknown) => caught); + + expect(error).toBeInstanceOf(InsufficientDiskSpaceError); + expect(runner.calls.map((call) => call.command)).toEqual(["xcrun"]); + releaseOther(); + }); }); it("provisions with the exact simctl argv and returns opaque iOS driver data", async () => { @@ -967,9 +1170,11 @@ function createDriver( clock = new FakeClock(), filesystem: Filesystem = new MemoryFilesystem(), onDiagnostic?: (diagnostic: ComponentInstallDiagnostic) => void, + diskSpaceGuard?: DiskSpaceGuard, ): IosSimctlDriver { return new IosSimctlDriver({ clock, + ...(diskSpaceGuard === undefined ? {} : { diskSpaceGuard }), filesystem, idGenerator: { generate: () => "device-1" }, ...(onDiagnostic === undefined ? {} : { onDiagnostic }), diff --git a/src/drivers/ios/index.ts b/src/drivers/ios/index.ts index 895968c..1daaf21 100644 --- a/src/drivers/ios/index.ts +++ b/src/drivers/ios/index.ts @@ -1,7 +1,7 @@ import { - assertDiskSpace, BootTimeoutError, type DeviceRequest, + DiskSpaceGuard, type Driver, type DriverCatalogEntry, type DriverDevice, @@ -74,6 +74,16 @@ export interface IosSimctlDriverOptions { readonly coreSimulatorRoot?: string; /** Per-download timeout; defaults to `downloads.timeoutMs`'s own default. */ readonly downloadTimeoutMs?: number; + /** + * Disk-space preflight, shared with every other driver that installs components -- a bare + * `assertDiskSpace` call only ever sees an instantaneous free-space reading, so two concurrent + * installs (this driver's and the Android driver's, or two of this driver's own) can each pass + * it and jointly overfill the volume neither alone would have. Defaults to a private, + * driver-local guard when omitted (tests, `SIMLOCK_DRIVERS_MODULE`); production wiring + * (`src/daemon/main.ts`) passes one instance to every driver so the tracking is actually + * shared. + */ + readonly diskSpaceGuard?: DiskSpaceGuard; readonly filesystem: Filesystem; readonly idGenerator: IdGenerator; /** @@ -123,7 +133,8 @@ export class IosSimctlDriver implements Driver { readonly platform = "ios" as const; readonly #clock: Clock; readonly #coreSimulatorRoot: string; - readonly #downloadLocks = new Map>(); + readonly #diskSpaceGuard: DiskSpaceGuard; + readonly #downloadLocks = new Map>(); readonly #downloadTimeoutMs: number; readonly #filesystem: Filesystem; readonly #idGenerator: IdGenerator; @@ -135,6 +146,7 @@ export class IosSimctlDriver implements Driver { constructor(options: IosSimctlDriverOptions) { this.#clock = options.clock; this.#coreSimulatorRoot = options.coreSimulatorRoot ?? "."; + this.#diskSpaceGuard = options.diskSpaceGuard ?? new DiskSpaceGuard(); this.#downloadTimeoutMs = options.downloadTimeoutMs ?? DEFAULT_DOWNLOAD_TIMEOUT_MS; this.#filesystem = options.filesystem; this.#idGenerator = options.idGenerator; @@ -144,7 +156,7 @@ export class IosSimctlDriver implements Driver { async resolveSpec( request: DeviceRequest, - options: { readonly allowDownload: boolean }, + options: { readonly allowDownload: boolean; readonly requesterId?: string }, ): Promise { this.#requireIosPlatform(request.platform); const catalog = await this.#loadCatalog(); @@ -166,11 +178,12 @@ export class IosSimctlDriver implements Driver { * *before* anything else -- an out-of-range request can never be fixed by downloading, so it * must never even reach the download decision. */ + // fallow-ignore-next-line complexity -- range/pairing checks, the download decision, and post-download verification are one resolution attempt. async #resolveExactRuntime( deviceType: DeviceType, osVersion: string, catalog: SimctlCatalog, - options: { readonly allowDownload: boolean }, + options: { readonly allowDownload: boolean; readonly requesterId?: string }, ): Promise { if (!isVersionInRange(osVersion, deviceType)) { throw new IosVersionOutOfRangeError(deviceType.name, osVersion, deviceType); @@ -200,19 +213,40 @@ export class IosSimctlDriver implements Driver { throw new IosDownloadFloorError(osVersion); } - await this.#downloadRuntime(osVersion, [ - "-downloadPlatform", - "iOS", - "-buildVersion", + const startedAt = await this.#downloadRuntime( osVersion, - ]); + ["-downloadPlatform", "iOS", "-buildVersion", osVersion], + options.requesterId, + ); + // `component.installed` is a verified fact, not "xcodebuild exited 0": re-scan the catalog + // and confirm the thing this request actually needed -- a runtime at this version, paired + // with this device type -- is now present before reporting success. Either failure mode + // reports `component-install-failed`, never `component-installed`. const refreshed = await this.#loadCatalog(); const runtime = findInstalledRuntime(refreshed, osVersion); if (runtime === undefined) { - throw new DriverCrashError( - `xcodebuild reported success but iOS ${osVersion} is still not installed`, + const message = `xcodebuild reported success but iOS ${osVersion} is still not installed`; + this.#reportVerificationFailure(osVersion, startedAt, message, options.requesterId); + throw new DriverCrashError(message); + } + // A version match alone is not enough: the same pairing check that gates an + // already-installed runtime above must also gate a freshly downloaded one -- a version can + // be on disk and still not pair with this specific device type. + if (!runtime.supportedDeviceTypeIds.has(deviceType.identifier)) { + this.#reportVerificationFailure( + osVersion, + startedAt, + `iOS ${osVersion} installed but does not pair with ${deviceType.name}`, + options.requesterId, ); + throw new IosRuntimeUnpairedError(deviceType.name, osVersion); } + this.#onDiagnostic?.({ + componentId: osVersion, + durationMs: this.#clock.now() - startedAt, + kind: "component-installed", + ...(options.requesterId === undefined ? {} : { requesterId: options.requesterId }), + }); return this.#commitResolution(deviceType, runtime); } @@ -222,10 +256,11 @@ export class IosSimctlDriver implements Driver { * installed runtime overall, which may have dropped this model (iOS 26 dropping iPhone * XS/XR support is the motivating case). */ + // fallow-ignore-next-line complexity -- the download-target decision and post-download verification are one resolution attempt. async #resolveDefaultRuntime( deviceType: DeviceType, catalog: SimctlCatalog, - options: { readonly allowDownload: boolean }, + options: { readonly allowDownload: boolean; readonly requesterId?: string }, ): Promise { const paired = pairedInstalledRuntime(catalog, deviceType); if (paired !== undefined) { @@ -240,14 +275,26 @@ export class IosSimctlDriver implements Driver { ); } + let componentId: string; + let startedAt: number; if (isUnboundedMax(deviceType.maxRuntimeVersion)) { // No upper bound on this model's pairing range: any released version works, so there is // nothing more specific to ask for than "latest". - await this.#downloadRuntime("latest", ["-downloadPlatform", "iOS"]); + componentId = "latest"; + startedAt = await this.#downloadRuntime( + componentId, + ["-downloadPlatform", "iOS"], + options.requesterId, + ); } else { const major = majorVersionString(deviceType.maxRuntimeVersion); + componentId = major; try { - await this.#downloadRuntime(major, ["-downloadPlatform", "iOS", "-buildVersion", major]); + startedAt = await this.#downloadRuntime( + major, + ["-downloadPlatform", "iOS", "-buildVersion", major], + options.requesterId, + ); } catch (error: unknown) { // A disk preflight failure or a typed "nothing to do here" (e.g. a concurrent caller's // RuntimeMissingError) is meaningful on its own and must reach the caller unchanged -- @@ -263,17 +310,48 @@ export class IosSimctlDriver implements Driver { } } + // Same verified-fact requirement as the exact-version path: only report `component-installed` + // once a paired runtime for this device type is actually present in a re-scanned catalog. const refreshed = await this.#loadCatalog(); const runtime = pairedInstalledRuntime(refreshed, deviceType); if (runtime === undefined) { - throw new DriverCrashError( + const message = `xcodebuild reported success but no installed iOS runtime pairs with ` + - `${deviceType.name} yet`, - ); + `${deviceType.name} yet`; + this.#reportVerificationFailure(componentId, startedAt, message, options.requesterId); + throw new DriverCrashError(message); } + this.#onDiagnostic?.({ + componentId, + durationMs: this.#clock.now() - startedAt, + kind: "component-installed", + ...(options.requesterId === undefined ? {} : { requesterId: options.requesterId }), + }); return this.#commitResolution(deviceType, runtime); } + /** + * Reports the terminal `component-install-failed` diagnostic for the "xcodebuild exited 0 but + * post-download verification didn't find what this request needed" case -- pairing failure or + * outright absence. `#installComponent` already reports `component-install-failed` for a + * nonzero xcodebuild exit; this covers the other way an install attempt can fail to produce a + * usable component. + */ + #reportVerificationFailure( + componentId: string, + startedAt: number, + message: string, + requesterId: string | undefined, + ): void { + this.#onDiagnostic?.({ + componentId, + durationMs: this.#clock.now() - startedAt, + error: message, + kind: "component-install-failed", + ...(requesterId === undefined ? {} : { requesterId }), + }); + } + #commitResolution(deviceType: DeviceType, runtime: Runtime): DeviceSpec { const spec: DeviceSpec = { model: deviceType.name, @@ -292,16 +370,22 @@ export class IosSimctlDriver implements Driver { * non-concurrent call starts a fresh attempt rather than replaying a stale result. `componentId` * is the runtime version being installed ("latest" for a bare `-downloadPlatform iOS`, the bare * major version for the bounded-default case) -- reported on `component.install-*`, never - * parsed back out of `args`. + * parsed back out of `args`. Resolves to the `started` timestamp on success rather than + * `void`: the caller needs it to compute an accurate `durationMs` once its own post-download + * catalog re-scan confirms (or fails to confirm) the component it actually needed. */ - async #downloadRuntime(componentId: string, args: readonly string[]): Promise { + async #downloadRuntime( + componentId: string, + args: readonly string[], + requesterId: string | undefined, + ): Promise { const key = args.join(""); const inFlight = this.#downloadLocks.get(key); if (inFlight !== undefined) { return inFlight; } - const promise = this.#installComponent(componentId, args).finally(() => { + const promise = this.#installComponent(componentId, args, requesterId).finally(() => { if (this.#downloadLocks.get(key) === promise) { this.#downloadLocks.delete(key); } @@ -311,35 +395,49 @@ export class IosSimctlDriver implements Driver { } /** - * Disk preflight, then `xcodebuild`, wrapped with `component.install-*` diagnostics. A - * preflight failure is reported before any diagnostic fires -- no install was actually - * attempted, so there is nothing to report as started or failed. + * Disk preflight (via the shared `DiskSpaceGuard`, released once `xcodebuild` settles either + * way), then `xcodebuild`, wrapped with `component.install-*` diagnostics. A preflight failure + * is reported before any diagnostic fires -- no install was actually attempted, so there is + * nothing to report as started or failed. */ - async #installComponent(componentId: string, args: readonly string[]): Promise { - await assertDiskSpace( + async #installComponent( + componentId: string, + args: readonly string[], + requesterId: string | undefined, + ): Promise { + const release = await this.#diskSpaceGuard.reserve( this.#filesystem, this.platform, IOS_RUNTIME_MIN_FREE_BYTES, this.#coreSimulatorRoot, ); - this.#onDiagnostic?.({ componentId, kind: "component-install-started" }); - const startedAt = this.#clock.now(); try { - await this.#xcodebuildOrThrow(args); - } catch (error: unknown) { this.#onDiagnostic?.({ componentId, - durationMs: this.#clock.now() - startedAt, - error: stableError(error), - kind: "component-install-failed", + kind: "component-install-started", + ...(requesterId === undefined ? {} : { requesterId }), }); - throw error; + const startedAt = this.#clock.now(); + try { + await this.#xcodebuildOrThrow(args); + } catch (error: unknown) { + this.#onDiagnostic?.({ + componentId, + durationMs: this.#clock.now() - startedAt, + error: stableError(error), + kind: "component-install-failed", + ...(requesterId === undefined ? {} : { requesterId }), + }); + throw error; + } + // No `component-installed` here: xcodebuild exiting 0 only means the tool claims success, + // not that the catalog now has what a specific caller needed (an exact version, or one + // that pairs with a specific device type). The caller re-scans and reports the terminal + // fact itself -- see `#reportVerificationFailure` and its call sites. + return startedAt; + } finally { + release(); } - this.#onDiagnostic?.({ - componentId, - durationMs: this.#clock.now() - startedAt, - kind: "component-installed", - }); } async #xcodebuildOrThrow(args: readonly string[]): Promise { @@ -807,8 +905,13 @@ function parseCatalog(value: unknown): SimctlCatalog { const deviceTypes = value.devicetypes.flatMap(parseDeviceType); const runtimes = value.runtimes.flatMap(parseRuntime); - if (deviceTypes.length === 0 || runtimes.length === 0) { - throw new DriverCrashError("Invalid simctl list JSON: no usable device types or runtimes"); + // Device types come from the Xcode install itself and are never empty on a working + // toolchain, so an empty list here means the JSON was malformed. Runtimes are different: a + // fresh Xcode with zero simulator runtimes installed is a normal, if unusual, starting state + // -- and it must be able to reach the download-latest path in `#resolveDefaultRuntime` rather + // than being rejected here before any resolution is attempted. + if (deviceTypes.length === 0) { + throw new DriverCrashError("Invalid simctl list JSON: no usable device types"); } return { deviceTypes, runtimes }; diff --git a/src/ports/index.ts b/src/ports/index.ts index af85faa..0eebdc7 100644 --- a/src/ports/index.ts +++ b/src/ports/index.ts @@ -34,13 +34,13 @@ export type { LogRecord } from "./logger.js"; export { CryptoIdGenerator, type IdGenerator } from "./id-generator.js"; export { FakeSystemStats, NodeSystemStats, type SystemStats } from "./system-stats.js"; export { FakeParentWatch, NodeParentWatch, type ParentWatch } from "./parent-watch.js"; -// fallow-ignore-next-line unused-type -- public handle contract returned by ParentWatch.watch(). export type { ParentWatchHandle } from "./parent-watch.js"; export { NodeProcessRunner, type ProcessHandle, type ProcessResult, type ProcessRunner, + type ProcessRunOptions, ScriptedProcessRunner, type ScriptedProcessExpectation, } from "./process-runner.js"; diff --git a/src/ports/process-runner.test.ts b/src/ports/process-runner.test.ts index f6ef90a..ac68d17 100644 --- a/src/ports/process-runner.test.ts +++ b/src/ports/process-runner.test.ts @@ -97,6 +97,25 @@ describe("NodeProcessRunner", () => { ).resolves.toEqual({ code: null, stderr: "", stdout: "" }); }); + // Real end-to-end coverage of the SIGTERM -> SIGKILL escalation: a child that installs a + // no-op SIGTERM handler survives the initial signal `run()` sends on timeout, so this only + // resolves at all if the follow-up SIGKILL actually lands once the grace period elapses. + // `SIGTERM_TO_SIGKILL_GRACE_MS` is a fixed 10s (see process-runner.ts), hence the generous + // per-test timeout below -- there is no faster way to prove the real runner's own timers + // actually escalate without mocking out `child_process`, which every other test in this + // `describe` block deliberately avoids. + it("escalates to SIGKILL when a timed-out process ignores SIGTERM", async () => { + const runner = new NodeProcessRunner(); + + await expect( + runner.run( + process.execPath, + ["-e", "process.on('SIGTERM', () => {}); setInterval(() => {}, 1_000)"], + { timeoutMs: 100 }, + ), + ).resolves.toEqual({ code: null, stderr: "", stdout: "" }); + }, 15_000); + it("settles wait() when a detached grandchild keeps the stdio pipe open after the process exits", async () => { const runner = new NodeProcessRunner(); // The child forks its own detached grandchild that inherits our pipe's write diff --git a/src/ports/process-runner.ts b/src/ports/process-runner.ts index a53475a..1dee32f 100644 --- a/src/ports/process-runner.ts +++ b/src/ports/process-runner.ts @@ -20,6 +20,13 @@ const EXIT_TO_CLOSE_GRACE_MS = 1_000; // may be incomplete; nothing else is waiting on it by then. const EXIT_TO_CLOSE_MAX_DEFERRAL_MS = 5_000; +// A hard bound on how long `run()` waits after a timeout-triggered SIGTERM before escalating to +// SIGKILL. A child that ignores SIGTERM (or is itself stuck in an uninterruptible wait) would +// otherwise hold the caller's `await process.wait()` open forever -- exactly the unbounded wait +// this constant exists to cap. Fixed rather than derived from `timeoutMs`: it is a +// termination-cleanup budget, not a scaled fraction of the operation's own timeout. +const SIGTERM_TO_SIGKILL_GRACE_MS = 10_000; + export interface ProcessRunOptions { readonly timeoutMs?: number; readonly env?: NodeJS.ProcessEnv; @@ -62,6 +69,7 @@ export class NodeProcessRunner implements ProcessRunner { options: ProcessRunOptions = {}, ): Promise { const process = this.spawn(command, args, options); + let killTimeout: NodeJS.Timeout | undefined; const timeout = options.timeoutMs === undefined ? undefined @@ -71,6 +79,18 @@ export class NodeProcessRunner implements ProcessRunner { } catch { // The child may have exited between the timer firing and the kill. } + // SIGTERM is a request, not a guarantee -- a child that ignores it (or is itself + // hung) must not be able to keep this `run()` call waiting indefinitely. Escalate to + // SIGKILL if it hasn't exited within the grace period; cleared below like `timeout` + // itself the moment `process.wait()` actually settles, so a child that does exit + // promptly after SIGTERM never sees the follow-up signal. + killTimeout = setTimeout(() => { + try { + process.kill("SIGKILL"); + } catch { + // The child may have exited between the timer firing and the kill. + } + }, SIGTERM_TO_SIGKILL_GRACE_MS); }, options.timeoutMs); try { @@ -79,6 +99,9 @@ export class NodeProcessRunner implements ProcessRunner { if (timeout !== undefined) { clearTimeout(timeout); } + if (killTimeout !== undefined) { + clearTimeout(killTimeout); + } } }