diff --git a/packages/platform-apple/src/core/app-launch.ts b/packages/platform-apple/src/core/app-launch.ts index 0d133040dd..f32f89ea72 100644 --- a/packages/platform-apple/src/core/app-launch.ts +++ b/packages/platform-apple/src/core/app-launch.ts @@ -291,6 +291,10 @@ function buildIosSimulatorLaunchArgs( options?: { launchConsole?: string; launchArgs?: string[]; terminateRunningApp?: boolean }, ): string[] { const args = ['launch']; + // `--console-pty` is the console mode this path needs: simctl writes the app's bytes to its own + // stdout through a PTY, which is what the caller redirects. `--stdout=`/`--stderr=` + // cannot substitute, because they resolve the given path inside the device's data container + // rather than on the host. Verified on Xcode 27.1. if (options?.launchConsole) args.push('--console-pty'); if (options?.terminateRunningApp) args.push('--terminate-running-process'); args.push(deviceId, bundleId); diff --git a/packages/platform-apple/src/runner/__tests__/runner-artifact-env.test.ts b/packages/platform-apple/src/runner/__tests__/runner-artifact-env.test.ts new file mode 100644 index 0000000000..5038a07fa7 --- /dev/null +++ b/packages/platform-apple/src/runner/__tests__/runner-artifact-env.test.ts @@ -0,0 +1,130 @@ +import assert from 'node:assert/strict'; +import path from 'node:path'; +import { test } from 'vitest'; +import { + buildRunnerSessionXctestrunDeviceCleanupPattern, + buildRunnerSessionXctestrunPathCleanupPattern, + buildRunnerSessionXctestrunSuffix, +} from '../runner-artifact-env.ts'; + +// Released daemons and clients pkill runner launches by these exact bytes, and the timeout client +// pins its own copy rather than deriving it, so it survives a rename here. Each expectation below +// is a literal on purpose: deriving it from this same module would survive any rename, which is +// precisely the change these lines exist to catch. + +const OWNED_ARTIFACT_PATH = + '/derived/Build/Products/AgentDeviceRunner.env.session-SIM-001-owner-4242-ab12cd34-8123.xctestrun'; + +test('the session suffix keeps the bytes and field order the matchers rely on', () => { + assert.equal( + buildRunnerSessionXctestrunSuffix({ + deviceId: 'SIM-001', + ownerToken: 'owner-4242-ab12cd34', + port: 8123, + }), + 'session-SIM-001-owner-4242-ab12cd34-8123', + ); +}); + +test('the lease cleanup pattern keeps the bytes released daemons pkilled', () => { + const owned = buildRunnerSessionXctestrunPathCleanupPattern(OWNED_ARTIFACT_PATH); + + assert.equal( + owned, + String.raw`AgentDeviceRunner\.env\.session-SIM-001-owner-4242-ab12cd34-8123\.xctestrun`, + ); + assert.equal(new RegExp(owned ?? '').test(argvFor(path.basename(OWNED_ARTIFACT_PATH))), true); + assert.equal( + new RegExp(owned ?? '').test(argvFor(sessionFileName('SIM-002', 'owner-4242-ab12cd34'))), + false, + 'another device must not match', + ); + assert.equal( + new RegExp(owned ?? '').test(argvFor(sessionFileName('SIM-001', 'owner-9999-ffee00'))), + false, + 'another owner must not match', + ); + assert.equal( + new RegExp(owned ?? '').test( + argvFor('AgentDeviceRunner.env.session-SIM-001-owner-4242-ab12cd34-8124.xctestrun'), + ), + false, + 'the same session on another port must not match', + ); +}); + +test('the device sweep pattern keeps selecting the pre-owner-token name and only that', () => { + // A reclaiming daemon with no lease to read knows the device but not the artifact, so it sweeps by + // device. The released bytes require a digit right after the device, which is the pre-owner-token + // spelling; an owner-token name needs its own path to be selected. + const sweep = buildRunnerSessionXctestrunDeviceCleanupPattern('SIM-002'); + + assert.equal(sweep, String.raw`AgentDeviceRunner\.env\.session-SIM-002-[0-9]`); + assert.equal( + new RegExp(sweep).test(argvFor('AgentDeviceRunner.env.session-SIM-002-8123.xctestrun')), + true, + ); + assert.equal( + new RegExp(sweep).test(argvFor(sessionFileName('SIM-002', 'owner-1-ff'))), + false, + 'a launch a lease still names must not fall to the device sweep', + ); +}); + +test('a flattened name field stays selectable', () => { + // A device id is caller-supplied and the writer flattens it onto disk, so both matchers have to + // survive that flattening: the recorded path already holds the bytes the launch carries, and the + // device sweep has to flatten the id it spells. + const suffix = buildRunnerSessionXctestrunSuffix({ + deviceId: 'SIM 01/x', + ownerToken: 'owner 7', + port: 80, + }); + + assert.equal(suffix, 'session-SIM_01_x-owner_7-80'); + const flattenedPath = `/derived/Build/Products/AgentDeviceRunner.env.${suffix}.xctestrun`; + assert.equal( + new RegExp(buildRunnerSessionXctestrunPathCleanupPattern(flattenedPath) ?? '').test( + argvFor(`AgentDeviceRunner.env.${suffix}.xctestrun`), + ), + true, + ); + // The pre-owner-token spelling of the same flattened device, which is the only name a sweep selects. + assert.equal( + new RegExp(buildRunnerSessionXctestrunDeviceCleanupPattern('SIM 01/x')).test( + argvFor('AgentDeviceRunner.env.session-SIM_01_x-8123.xctestrun'), + ), + true, + ); +}); + +test('a detached lease still selects the launch it started', () => { + // Detaching rewrites `ownerToken` to `detached-` while the launch keeps running under the + // name the writer gave it. The bytes a token-derived pattern would have carried are spelled here + // literally: they select a file that never existed, which is the gap following the recorded path + // closes. + const detachedTokenPattern = String.raw`AgentDeviceRunner\.env\.session-SIM-001-detached-owner-4242-ab12cd34-`; + const argv = argvFor(path.basename(OWNED_ARTIFACT_PATH)); + + assert.equal(new RegExp(detachedTokenPattern).test(argv), false); + assert.equal( + new RegExp(buildRunnerSessionXctestrunPathCleanupPattern(OWNED_ARTIFACT_PATH) ?? '').test(argv), + true, + ); +}); + +test('a lease artifact path outside the runner session name declines to a pattern', () => { + // A basename like `runner.xctestrun` would escape into a pattern loose enough to signal unrelated + // xcodebuilds, so the caller has to fall back to the device sweep. + assert.equal(buildRunnerSessionXctestrunPathCleanupPattern('/tmp/runner.xctestrun'), undefined); + assert.equal(buildRunnerSessionXctestrunPathCleanupPattern(''), undefined); + assert.equal(buildRunnerSessionXctestrunPathCleanupPattern(undefined), undefined); +}); + +function sessionFileName(deviceId: string, ownerToken: string): string { + return `AgentDeviceRunner.env.${buildRunnerSessionXctestrunSuffix({ deviceId, ownerToken, port: 8123 })}.xctestrun`; +} + +function argvFor(fileName: string): string { + return `xcodebuild test-without-building -xctestrun /derived/Build/Products/${fileName}`; +} diff --git a/packages/platform-apple/src/runner/__tests__/runner-disposal.test.ts b/packages/platform-apple/src/runner/__tests__/runner-disposal.test.ts index ffbba4293f..b658163c14 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-disposal.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-disposal.test.ts @@ -15,8 +15,13 @@ vi.mock('../runner-io.ts', async (importOriginal) => { return { ...actual, cleanupTempFile: mockCleanupTempFile }; }); -import { abortRunnerSessionsAndPrepProcesses, disposeRunnerSession } from '../runner-disposal.ts'; import { + abortRunnerSessionsAndPrepProcesses, + disposeRunnerSession, + runnerLeaseCleanupAdapter, +} from '../runner-disposal.ts'; +import { + buildDetachedRunnerLease, currentRunnerLeaseOwnerToken, releaseRunnerLease, withRunnerLeaseLock, @@ -182,6 +187,57 @@ test('disposal serializes behind a successor reclaim window and never terminates expect(currentRunnerLeaseOwnerToken(IOS_SIMULATOR.id)).toBe('owner-toctou-successor'); }); +test('a leased cleanup selects the launch named by the artifact the lease recorded', async () => { + // The launch is found by matching its argv, and a lease knows the artifact that launch was + // started with. Detaching is why following the path rather than the token matters: it rewrites + // `ownerToken` to `detached-` while the handed-over xcodebuild keeps the name the writer + // gave it, so a pattern rebuilt from the token names a file that never existed. + const detached = buildDetachedRunnerLease( + makeRunnerLease({ deviceId: IOS_SIMULATOR.id, ownerToken: 'owner-4242-ab12cd34' }), + ); + expect(detached.ownerToken).toBe('detached-owner-4242-ab12cd34'); + + await runnerLeaseCleanupAdapter.cleanupRunnerXcodebuildProcesses({ + deviceId: detached.deviceId, + xctestrunPath: detached.xctestrunPath, + }); + + const pattern = runnerXcodebuildPkillPatterns()[0]; + expect(pattern).toBeDefined(); + const selects = (xctestrunPath: string): boolean => + new RegExp(pattern ?? '').test(runnerLaunchArgv(xctestrunPath)); + expect(selects(detached.xctestrunPath)).toBe(true); + // A launch this lease does not name is somebody else's, and must stay untouched. + expect(selects('/tmp/other/AgentDeviceRunner.xctestrun')).toBe(false); +}); + +test('a cleanup with no recorded artifact sweeps that device launches only', async () => { + // A reclaim with no lease to read knows the device and nothing else, so it keeps the released + // pre-owner-token bytes and stays scoped to that device instead of every xcodebuild on the host. + await runnerLeaseCleanupAdapter.cleanupRunnerXcodebuildProcesses({ deviceId: 'SIM-OTHER' }); + + const pattern = runnerXcodebuildPkillPatterns()[0]; + expect(pattern).toBeDefined(); + const selects = (fileName: string): boolean => + new RegExp(pattern ?? '').test(runnerLaunchArgv(`/tmp/${fileName}`)); + expect(selects('AgentDeviceRunner.env.session-SIM-OTHER-8123.xctestrun')).toBe(true); + expect(selects('AgentDeviceRunner.env.session-SIM-VICTIM-8123.xctestrun')).toBe(false); + // A launch a lease still names is not this sweep's to take: it is reached by its own lease. + expect( + selects('AgentDeviceRunner.env.session-SIM-OTHER-owner-4242-ab12cd34-8123.xctestrun'), + ).toBe(false); +}); + +function runnerXcodebuildPkillPatterns(): string[] { + return mockRunAppleToolCommand.mock.calls + .filter(([tool, args]) => tool === 'pkill' && (args as string[]).includes('-f')) + .map(([, args]) => String((args as string[])[2])); +} + +function runnerLaunchArgv(xctestrunPath: string): string { + return `xcodebuild test-without-building -xctestrun ${xctestrunPath}`; +} + function simulatorTerminateCalls(): unknown[] { return mockRunXcrun.mock.calls.filter(([args]) => (args as string[]).includes('terminate')); } diff --git a/packages/platform-apple/src/runner/__tests__/runner-lease-claim-takeover.test.ts b/packages/platform-apple/src/runner/__tests__/runner-lease-claim-takeover.test.ts index ca281a87bf..4659e71dcb 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-lease-claim-takeover.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-lease-claim-takeover.test.ts @@ -1,5 +1,6 @@ import assert from 'node:assert/strict'; import fs from 'node:fs'; +import path from 'node:path'; import { afterEach, beforeEach, test } from 'vitest'; import type { DeviceInfo } from '@agent-device/kernel/device'; import { appleRunnerTestHost } from '../test-host.ts'; @@ -55,8 +56,11 @@ function recordingCleanupAdapter(): RunnerLeaseCleanupAdapter & { calls: string[ cleanupRunnerProcessTree: async (_pid, signal) => { calls.push(`process-tree:${signal}`); }, - cleanupRunnerXcodebuildProcesses: async (_deviceId, ownerToken) => { - calls.push(`xcodebuild:${ownerToken ?? 'any'}`); + cleanupRunnerXcodebuildProcesses: async (target) => { + const basename = target.xctestrunPath + ? `:${path.basename(target.xctestrunPath)}` + : ':device-sweep'; + calls.push(`xcodebuild:${target.deviceId}${basename}`); }, cleanupTempFile: (filePath) => { calls.push(`temp:${filePath}`); @@ -78,7 +82,13 @@ test('device-claim authority reclaims a live foreign claim-aware lease', async ( await prepareRunnerLeaseForStartup(device, cleanup); - assert.ok(cleanup.calls.includes('xcodebuild:owner-foreign-live')); + // The takeover kills the launch the lease named, so it must carry the recorded artifact path + // rather than anything derived from the owner token it is preempting. + assert.ok( + cleanup.calls.includes( + 'xcodebuild:claim-takeover-sim:AgentDeviceRunner.env.session-claim-takeover-sim-owner-foreign-live-8123.xctestrun', + ), + ); // The probe must receive the full device identity, never a bare id: claim // ownership is canonical family/OS/id, and a same-id claim from another // platform family grants nothing. @@ -89,7 +99,7 @@ test('device-claim authority reclaims a live foreign claim-aware lease', async ( // sees an empty store instead of the foreign owner. const emptyCleanup = recordingCleanupAdapter(); await prepareRunnerLeaseForStartup(device, emptyCleanup); - assert.ok(emptyCleanup.calls.includes('xcodebuild:any')); + assert.ok(emptyCleanup.calls.includes(`xcodebuild:${device.id}:device-sweep`)); }); test('a claim-aware lease still refuses without device-claim authority', async () => { diff --git a/packages/platform-apple/src/runner/__tests__/runner-session.test.ts b/packages/platform-apple/src/runner/__tests__/runner-session.test.ts index aa96594fec..b7ea9f0c01 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-session.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-session.test.ts @@ -132,6 +132,7 @@ import { writeRunnerLease, type RunnerLease, type RunnerLeaseCleanupAdapter, + type RunnerXcodebuildCleanupTarget, } from '../runner-lease.ts'; // Test-only stand-in for the daemon's own runtime lease-owner-state-dir @@ -741,13 +742,13 @@ test('runner session startup reclaims dead foreign runner lease before launching // separate adapter call and must keep running either way. function makeRecordingCleanupAdapter() { const treeKills: Array<{ pid: number | undefined; signal: string }> = []; - const xcodebuildCleanups: Array<{ deviceId: string; ownerToken: string | undefined }> = []; + const xcodebuildCleanups: RunnerXcodebuildCleanupTarget[] = []; const adapter: RunnerLeaseCleanupAdapter = { async cleanupRunnerProcessTree(pid, signal) { treeKills.push({ pid, signal }); }, - async cleanupRunnerXcodebuildProcesses(deviceId, ownerToken) { - xcodebuildCleanups.push({ deviceId, ownerToken }); + async cleanupRunnerXcodebuildProcesses(target) { + xcodebuildCleanups.push(target); }, cleanupTempFile() {}, }; @@ -783,9 +784,8 @@ test('stale-lease cleanup does not signal a recycled runner pid (start time mism { pid: undefined, signal: 'SIGTERM' }, { pid: undefined, signal: 'SIGKILL' }, ]); - assert.deepEqual(xcodebuildCleanups, [ - { deviceId: device.id, ownerToken: 'owner-dead-recycled' }, - ]); + const sweptDeviceIds = xcodebuildCleanups.map((target) => target.deviceId); + assert.deepEqual(sweptDeviceIds, [device.id]); }); test('stale-lease cleanup signals the runner pid when its start time still matches', async () => { diff --git a/packages/platform-apple/src/runner/__tests__/runner-xctestrun.test.ts b/packages/platform-apple/src/runner/__tests__/runner-xctestrun.test.ts index 79848916ba..be8a4d2ba6 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-xctestrun.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-xctestrun.test.ts @@ -6,6 +6,10 @@ import os from 'node:os'; import path from 'node:path'; import { pathToFileURL } from 'node:url'; import { mkdtempForTestSync } from './tmp-dir.ts'; +import { + buildRunnerSessionXctestrunPathCleanupPattern, + buildRunnerSessionXctestrunSuffix, +} from '../runner-artifact-env.ts'; import { appleRunnerTestHost } from '../test-host.ts'; import type { ExecOptions, ExecResult } from '@agent-device/host-kit/command'; @@ -358,6 +362,46 @@ test('prepareXctestrunWithEnv writes env overlays into configured env dir', asyn }); }); +test('the session xctestrun the writer builds is found by the cleanup matcher', async () => { + // The launch is killed by `pkill -f` on a pattern the writer module itself builds, and the + // daemon-client sweep pins its own looser copy of these bytes. Binding writer to pattern and to + // those pinned bytes here is what makes a rename that keeps each side self-consistent fail. + await withTempDir('runner-xctestrun-identity-', async (root) => { + const xctestrunPath = path.join(root, 'AgentDeviceRunner.xctestrun'); + fs.writeFileSync( + xctestrunPath, + JSON.stringify({ + TestConfigurations: [{ TestTargets: [{ TestBundlePath: 'AgentDeviceRunnerUITests' }] }], + }), + ); + appleRunnerTestHost.update({ runAppleToolCommand: fakeXctestrunPlutilToolCommand() }); + const suffix = buildRunnerSessionXctestrunSuffix({ + deviceId: 'SIM-001', + ownerToken: 'owner-4242-ab12cd34', + port: 8123, + }); + + const prepared = await prepareXctestrunWithEnv(xctestrunPath, runnerPortEnv, suffix); + const argv = `xcodebuild test-without-building -xctestrun ${prepared.xctestrunPath}`; + + // The bytes the timeout sweep pins literally, so the sweep cannot drift off the writer's name. + assert.match(argv, new RegExp(String.raw`xcodebuild .*AgentDeviceRunner\.env\.session-`)); + // The lease-backed pattern is spelled from the path the writer returned, so pin those bytes + // literally rather than comparing the pattern to the path it was built from: a rename that keeps + // both sides self-consistent must still fail here. + assert.equal( + buildRunnerSessionXctestrunPathCleanupPattern(prepared.xctestrunPath), + String.raw`AgentDeviceRunner\.env\.session-SIM-001-owner-4242-ab12cd34-8123\.xctestrun`, + ); + assert.match( + argv, + new RegExp( + String.raw`xcodebuild.*test-without-building.*AgentDeviceRunner\.env\.session-SIM-001-owner-4242-ab12cd34-8123\.xctestrun`, + ), + ); + }); +}); + test('prepareXctestrunWithEnv leaves unrelated targets without capture policy', async () => { await withTempDir('runner-xctestrun-policy-', async (root) => { const xctestrunPath = path.join(root, 'AgentDeviceRunner.xctestrun'); diff --git a/packages/platform-apple/src/runner/runner-artifact-env.ts b/packages/platform-apple/src/runner/runner-artifact-env.ts index 1f4858ceea..b970887f62 100644 --- a/packages/platform-apple/src/runner/runner-artifact-env.ts +++ b/packages/platform-apple/src/runner/runner-artifact-env.ts @@ -6,6 +6,82 @@ import { requireExecSuccess, runAppleToolCommand } from './host.ts'; /** The xctestrun plist spells environment blocks as fully defined string maps. */ type EnvMap = Record; +/** + * The name of the per-session `.xctestrun` a runner launch is started with, and the patterns that + * find that launch again by name. + * + * The name reaches `xcodebuild`'s argv as `-xctestrun /.xctestrun`, and runner cleanup + * selects the live launch by matching that argv with `pkill -f`: a daemon kills the launches it + * started itself, and a daemon reclaiming a lease kills another daemon's. The name is therefore a + * durable process-identity contract, not a scratch filename: renaming it, or reordering the + * suffix, orphans the xcodebuilds an earlier version started. The daemon-client timeout sweep + * ships separately and cannot follow a rename, so it pins these bytes as a literal instead of + * deriving them; its test proves the literal still selects both name eras. + */ +const RUNNER_SESSION_XCTESTRUN_STEM = 'AgentDeviceRunner.env'; + +/** First field of the session suffix; {@link prepareXctestrunWithEnv} joins it to the stem. */ +const SESSION_FIELD_PREFIX = 'session'; + +/** + * The session-name prefix as extended-regular-expression source for `pkill -f`, including the + * separator that follows it. Spelled from the same parts the writer joins, so the filename and its + * matchers cannot disagree about whether a dot is literal. + */ +const RUNNER_SESSION_XCTESTRUN_NAME_PATTERN = `${escapeForExtendedRegex( + `${RUNNER_SESSION_XCTESTRUN_STEM}.${SESSION_FIELD_PREFIX}`, +)}-`; + +/** + * Builds the suffix the session writes after the stem: `session---`. + * Cleanup matches on that exact field order, and the sanitization is what keeps a device id holding + * a path separator from naming a file the matcher can never select. + */ +export function buildRunnerSessionXctestrunSuffix( + params: Readonly<{ deviceId: string; ownerToken: string; port: number }>, +): string { + return sanitizeRunnerSessionNameField( + `${SESSION_FIELD_PREFIX}-${params.deviceId}-${params.ownerToken}-${params.port}`, + ); +} + +/** + * The `pkill -f` pattern selecting one device's runner launches, for a caller that has no lease to + * read and therefore knows only the device. The device is followed by the port — the pre-owner-token + * spelling, kept matchable because a released version may still hold such a launch. + */ +export function buildRunnerSessionXctestrunDeviceCleanupPattern(deviceId: string): string { + return `${RUNNER_SESSION_XCTESTRUN_NAME_PATTERN}${escapeForExtendedRegex( + sanitizeRunnerSessionNameField(deviceId), + )}-[0-9]`; +} + +/** + * The `pkill -f` pattern selecting the launch a lease describes, from the xctestrun path that lease + * recorded. The basename is what the launch carries in argv, so a lease that no longer spells its + * own name — a detached lease rewrites `ownerToken` — still selects exactly the launch it started. + * + * Returns `undefined` when the recorded path does not name a runner session artifact: such a + * basename could be any short string, and a pattern that loose would signal unrelated xcodebuilds. + * Callers fall back to {@link buildRunnerSessionXctestrunDeviceCleanupPattern} for those leases. + */ +export function buildRunnerSessionXctestrunPathCleanupPattern( + xctestrunPath: string | undefined, +): string | undefined { + const basename = xctestrunPath ? path.basename(xctestrunPath) : ''; + if (!basename.startsWith(`${RUNNER_SESSION_XCTESTRUN_STEM}.`)) return undefined; + return escapeForExtendedRegex(basename); +} + +/** Characters a session name may carry; anything else is flattened, as the filesystem does. */ +function sanitizeRunnerSessionNameField(value: string): string { + return value.replaceAll(/[^a-zA-Z0-9._-]/g, '_'); +} + +function escapeForExtendedRegex(value: string): string { + return value.replaceAll(/[.*+?^${}()|[\]\\]/g, String.raw`\$&`); +} + const RUNNER_XCTESTRUN_CAPTURE_OPTIONS = { PreferredScreenCaptureFormat: 'screenshots', SystemAttachmentLifetime: 'keepNever', @@ -44,14 +120,18 @@ export async function prepareXctestrunWithEnv( const configuredEnvDir = options.iosXctestEnvDir?.trim(); const dir = configuredEnvDir ? path.resolve(configuredEnvDir) : path.dirname(xctestrunPath); fs.mkdirSync(dir, { recursive: true }); - const safeSuffix = suffix.replaceAll(/[^a-zA-Z0-9._-]/g, '_'); - const tmpJsonPath = path.join(dir, `AgentDeviceRunner.env.${safeSuffix}.json`); - const tmpXctestrunPath = path.join(dir, `AgentDeviceRunner.env.${safeSuffix}.xctestrun`); + const safeSuffix = sanitizeRunnerSessionNameField(suffix); + const tmpJsonPath = path.join(dir, `${RUNNER_SESSION_XCTESTRUN_STEM}.${safeSuffix}.json`); + const tmpXctestrunPath = path.join( + dir, + `${RUNNER_SESSION_XCTESTRUN_STEM}.${safeSuffix}.xctestrun`, + ); const parsed = await readXctestrunPlist(xctestrunPath); visitXctestrunTargets(parsed, (target) => mergeEnvIntoXctestrunTarget(target, envVars)); - // Xcode 26.2 can emit attachment lifetime values that differ from the test plan, - // so normalize the per-session xctestrun immediately before test-without-building. + // Xcode re-synthesizes these keys from its own defaults, not the test plan: building this + // runner's plan (which sets keepNever) still yields SystemAttachmentLifetime=deleteOnSuccess on + // Xcode 26.2 and 27.1, so a launch that skipped this rewrite would keep a screenshot per test. applyRunnerXctestrunCapturePolicy(parsed); await writeXctestrunPlist(parsed, tmpJsonPath, tmpXctestrunPath); diff --git a/packages/platform-apple/src/runner/runner-disposal.ts b/packages/platform-apple/src/runner/runner-disposal.ts index fdfc5c08b2..1dc18c2d5e 100644 --- a/packages/platform-apple/src/runner/runner-disposal.ts +++ b/packages/platform-apple/src/runner/runner-disposal.ts @@ -9,6 +9,10 @@ import { runXcrun, } from './host.ts'; import type { ExecBackgroundResult } from '@agent-device/host-kit/command'; +import { + buildRunnerSessionXctestrunDeviceCleanupPattern, + buildRunnerSessionXctestrunPathCleanupPattern, +} from './runner-artifact-env.ts'; import { isMacOs, type DeviceInfo } from '@agent-device/kernel/device'; import { cleanupTempFile } from './runner-io.ts'; import { waitForRunner } from './runner-startup-transport.ts'; @@ -19,6 +23,7 @@ import { releaseRunnerLease, withRunnerLeaseLock, type RunnerLeaseCleanupAdapter, + type RunnerXcodebuildCleanupTarget, } from './runner-lease.ts'; import { IOS_RUNNER_CONTAINER_BUNDLE_IDS, runnerPrepProcesses } from './runner-xctestrun.ts'; import { advanceRunnerSessionState, type RunnerSession } from './runner-session-types.ts'; @@ -327,13 +332,12 @@ async function killRunnerProcessTree( } catch {} } -async function killRunnerXcodebuildProcesses( - deviceId: string, - ownerToken: string | undefined, -): Promise { - const pattern = ownerToken - ? `xcodebuild.*test-without-building.*AgentDeviceRunner\\.env\\.session-${escapeRegex(deviceId)}-${escapeRegex(ownerToken)}-` - : `xcodebuild.*test-without-building.*AgentDeviceRunner\\.env\\.session-${escapeRegex(deviceId)}-[0-9]`; +async function killRunnerXcodebuildProcesses(target: RunnerXcodebuildCleanupTarget): Promise { + const { deviceId } = target; + const pattern = `xcodebuild.*test-without-building.*${ + buildRunnerSessionXctestrunPathCleanupPattern(target.xctestrunPath) ?? + buildRunnerSessionXctestrunDeviceCleanupPattern(deviceId) + }`; for (const signal of ['TERM', 'KILL'] as const) { try { await runAppleToolCommand('pkill', [`-${signal}`, '-f', pattern], { @@ -353,7 +357,3 @@ async function killRunnerXcodebuildProcesses( } } } - -function escapeRegex(value: string): string { - return value.replaceAll(/[.*+?^${}()|[\]\\]/g, String.raw`\$&`); -} diff --git a/packages/platform-apple/src/runner/runner-lease.ts b/packages/platform-apple/src/runner/runner-lease.ts index 4110631f68..a1f33392c6 100644 --- a/packages/platform-apple/src/runner/runner-lease.ts +++ b/packages/platform-apple/src/runner/runner-lease.ts @@ -95,9 +95,20 @@ type RunnerLeaseRequiredFields = Pick< 'createdAtMs' | 'jsonPath' | 'ownerPid' | 'ownerToken' | 'port' | 'sessionId' | 'xctestrunPath' >; +/** + * Which runner xcodebuild launches a cleanup may signal. + * + * With `xctestrunPath` — the path a lease recorded — the sweep is scoped to the one launch that + * artifact names. Without it the caller only knows the device, so the sweep covers that device's + * launches and must be a reclaim, never a stop of a session this daemon still considers live. + */ +export type RunnerXcodebuildCleanupTarget = Readonly< + { deviceId: string } & ({ xctestrunPath: string } | { xctestrunPath?: undefined }) +>; + export type RunnerLeaseCleanupAdapter = { cleanupRunnerProcessTree(pid: number | undefined, signal: 'SIGTERM' | 'SIGKILL'): Promise; - cleanupRunnerXcodebuildProcesses(deviceId: string, ownerToken: string | undefined): Promise; + cleanupRunnerXcodebuildProcesses(target: RunnerXcodebuildCleanupTarget): Promise; cleanupTempFile(filePath: string): void; }; @@ -180,7 +191,7 @@ export async function prepareRunnerLeaseForStartup( const deviceId = device.id; const state = classifyRunnerLease(readRunnerLease(deviceId)); if (state.type === 'empty') { - await cleanup.cleanupRunnerXcodebuildProcesses(deviceId, undefined); + await cleanup.cleanupRunnerXcodebuildProcesses({ deviceId }); return; } if (state.type === 'busy') { @@ -551,7 +562,10 @@ async function cleanupLeasedRunnerProcesses( }, }); await cleanup.cleanupRunnerProcessTree(resolveVerifiedLeaseRunnerPid(lease), 'SIGTERM'); - await cleanup.cleanupRunnerXcodebuildProcesses(lease.deviceId, lease.ownerToken); + await cleanup.cleanupRunnerXcodebuildProcesses({ + deviceId: lease.deviceId, + xctestrunPath: lease.xctestrunPath, + }); await cleanup.cleanupRunnerProcessTree(resolveVerifiedLeaseRunnerPid(lease), 'SIGKILL'); cleanup.cleanupTempFile(lease.xctestrunPath); cleanup.cleanupTempFile(lease.jsonPath); diff --git a/packages/platform-apple/src/runner/runner-process-launch.ts b/packages/platform-apple/src/runner/runner-process-launch.ts index 8450942fd4..cafee10e78 100644 --- a/packages/platform-apple/src/runner/runner-process-launch.ts +++ b/packages/platform-apple/src/runner/runner-process-launch.ts @@ -85,6 +85,10 @@ export function launchRunnerProcess(input: LaunchRunnerProcessInput): LaunchedRu ], { allowFailure: true, + // xcodebuild does not forward its own environment to the test runner: per xcodebuild(1), + // only names prefixed `TEST_RUNNER_` cross that boundary, with the prefix stripped. This + // entry is visible to xcodebuild alone, and the runner reads its port from the session + // xctestrun's EnvironmentVariables instead. env: { ...process.env, AGENT_DEVICE_RUNNER_PORT: String(input.port) }, detached: true, signal: input.signal, diff --git a/packages/platform-apple/src/runner/runner-session.ts b/packages/platform-apple/src/runner/runner-session.ts index 5319e7f0e3..576dadd7d0 100644 --- a/packages/platform-apple/src/runner/runner-session.ts +++ b/packages/platform-apple/src/runner/runner-session.ts @@ -60,6 +60,7 @@ import { writeRunnerLease, } from './runner-lease.ts'; import { isIosRunnerDetachEnabled, tryAdoptRunnerSessionFromLease } from './runner-adoption.ts'; +import { buildRunnerSessionXctestrunSuffix } from './runner-artifact-env.ts'; import { abortRunnerSessionsAndPrepProcesses, cleanupOwnedIosRunnerLease, @@ -276,7 +277,11 @@ async function startRunnerSessionWithLease( await prepareXctestrunWithEnv( xctestrunArtifact.xctestrunPath, { AGENT_DEVICE_RUNNER_PORT: String(port) }, - `session-${device.id}-${runnerOwnerToken()}-${port}`, + buildRunnerSessionXctestrunSuffix({ + deviceId: device.id, + ownerToken: runnerOwnerToken(), + port, + }), { iosXctestEnvDir: options.iosXctestEnvDir }, ), )); diff --git a/src/daemon-client/__tests__/daemon-client-timeout-route.test.ts b/src/daemon-client/__tests__/daemon-client-timeout-route.test.ts index 9b2fe5b6d1..195960e862 100644 --- a/src/daemon-client/__tests__/daemon-client-timeout-route.test.ts +++ b/src/daemon-client/__tests__/daemon-client-timeout-route.test.ts @@ -154,6 +154,27 @@ test('socket timeout: pkill cleanup still runs for a declared non-Apple platform // design that skips cleanup based on the declared flag would fail this. const pkillCalls = mockRunCmdSync.mock.calls.filter(([cmd]) => cmd === 'pkill'); assert.equal(pkillCalls.length, 3); + + // The session-xctestrun pattern is pinned by bytes, not derived from the runner's writer module: + // a client version in the field already pkills this exact string, and it must keep selecting + // launches that older writers named, since it cannot know which version started a timed-out + // launch. Deriving it would move this sweep off those names on any rename. + const sessionPattern = pkillCalls + .map(([, args]) => String(args?.[1])) + .find((pattern) => pattern.includes('session')); + assert.equal(sessionPattern, String.raw`xcodebuild .*AgentDeviceRunner\.env\.session-`); + assert.equal( + new RegExp(sessionPattern).test( + 'xcodebuild test-without-building -xctestrun /d/AgentDeviceRunner.env.session-SIM-1-owner-1-ff-8123.xctestrun', + ), + true, + ); + assert.equal( + new RegExp(sessionPattern).test( + 'xcodebuild test-without-building -xctestrun /d/AgentDeviceRunner.env.session-SIM-1-8123.xctestrun', + ), + true, + ); }); test('http timeout: pkill cleanup still runs for an undeclared platform (unknown-session case) that terminates nothing, and the hint stays platform-neutral', async () => { diff --git a/src/daemon-client/daemon-client-timeout.ts b/src/daemon-client/daemon-client-timeout.ts index 4fd9ca802a..e6c1442eb4 100644 --- a/src/daemon-client/daemon-client-timeout.ts +++ b/src/daemon-client/daemon-client-timeout.ts @@ -16,6 +16,10 @@ import { const IOS_RUNNER_XCODEBUILD_KILL_PATTERNS = [ 'xcodebuild .*AgentDeviceRunnerUITests/RunnerTests/testCommand', + // A client in the field already pkills these exact bytes. This sweep ships separately from the + // runner and cannot know which version wrote a timed-out launch, so it never follows a rename: + // it must keep matching the names older writers used. The literal stays pinned here rather than + // derived from `runner-artifact-env.ts`, which builds only today's session name. String.raw`xcodebuild .*AgentDeviceRunner\.env\.session-`, String.raw`xcodebuild build-for-testing .*apple/runner/AgentDeviceRunner/AgentDeviceRunner\.xcodeproj`, ];