From 0683a3d4646370cf9c13161a7538f601956b0e50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 25 Sep 2026 20:05:24 +0200 Subject: [PATCH 1/3] refactor(ios-runner): name the runner session xctestrun as a process-identity contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A runner launch is killed by matching `xcodebuild`'s argv with `pkill -f`, and that argv carries the per-session xctestrun's filename. The filename is therefore a process-identity contract, but it was spelled three times: inline in the session, inline in disposal, and again in the daemon-client timeout sweep. Disposal escaped the device id for regex without flattening it the way the writer's filesystem sanitization does, so a device id needing flattening built a cleanup pattern that could not match the file the writer had already written. Name it once. `runner-artifact-env.ts` now owns the stem, the suffix field order, the sanitization, and the `pkill -f` pattern derived from those same parts, and the writer, the session, and disposal all go through it. The name lives in the writer rather than a shared contracts module because the timeout sweep must not follow a rename: it ships separately, cannot know which version named a timed-out launch, and has to keep matching the names older writers used. So the sweep keeps a pinned literal, and its test proves those bytes still select both the owner-token and the pre-owner-token spelling. Placing the name in the module the Apple façades already evaluate also holds the eager-closure budget (#1960) at zero new edges. Verified byte-for-byte against the previous patterns for concrete, host-style, and flattened device ids. The only divergence is the flattened case, where the old pattern failed to match its own file. Also record three tooling constraints read from `xcodebuild(1)` and `simctl help`, and checked on Xcode 27.1: only `TEST_RUNNER_`-prefixed names cross into the test runner; `simctl launch --stdout/--stderr` resolve their paths inside the device's data container, so `--console-pty` is the console mode this host path can use; and xcodebuild re-synthesizes the attachment-lifetime keys from its own defaults, since a plan setting `keepNever` still builds an xctestrun carrying `SystemAttachmentLifetime=deleteOnSuccess`. --- .../platform-apple/src/core/app-launch.ts | 4 + .../__tests__/runner-artifact-env.test.ts | 88 +++++++++++++++++++ .../runner/__tests__/runner-xctestrun.test.ts | 40 +++++++++ .../src/runner/runner-artifact-env.ts | 79 +++++++++++++++-- .../src/runner/runner-disposal.ts | 11 +-- .../src/runner/runner-process-launch.ts | 4 + .../src/runner/runner-session.ts | 7 +- .../daemon-client-timeout-route.test.ts | 21 +++++ src/daemon-client/daemon-client-timeout.ts | 4 + 9 files changed, 245 insertions(+), 13 deletions(-) create mode 100644 packages/platform-apple/src/runner/__tests__/runner-artifact-env.test.ts 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..95956b665e --- /dev/null +++ b/packages/platform-apple/src/runner/__tests__/runner-artifact-env.test.ts @@ -0,0 +1,88 @@ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import { + buildRunnerSessionXctestrunCleanupPattern, + 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. + +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 owner cleanup pattern keeps the bytes released daemons pkilled', () => { + const owned = buildRunnerSessionXctestrunCleanupPattern({ + deviceId: 'SIM-001', + ownerToken: 'owner-4242-ab12cd34', + }); + + assert.equal(owned, String.raw`AgentDeviceRunner\.env\.session-SIM-001-owner-4242-ab12cd34-`); + assert.equal( + new RegExp(owned).test(argvFor(sessionFileName('SIM-001', 'owner-4242-ab12cd34'))), + 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', + ); +}); + +test('a tokenless cleanup 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 owner, 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 token to be selected. + const tokenless = buildRunnerSessionXctestrunCleanupPattern({ deviceId: 'SIM-002' }); + + assert.equal(tokenless, String.raw`AgentDeviceRunner\.env\.session-SIM-002-[0-9]`); + assert.equal( + new RegExp(tokenless).test(argvFor('AgentDeviceRunner.env.session-SIM-002-8123.xctestrun')), + true, + ); + assert.equal( + new RegExp(tokenless).test(argvFor(sessionFileName('SIM-002', 'owner-1-ff'))), + false, + ); +}); + +test('a name field is sanitized the way the filesystem writer sanitizes it', () => { + // A device id is caller-supplied and the writer flattens it onto disk, so the matcher has to + // flatten it too or it selects a name the launch never had. + const suffix = buildRunnerSessionXctestrunSuffix({ + deviceId: 'SIM 01/x', + ownerToken: 'owner 7', + port: 80, + }); + + assert.equal(suffix, 'session-SIM_01_x-owner_7-80'); + assert.equal( + new RegExp( + buildRunnerSessionXctestrunCleanupPattern({ deviceId: 'SIM 01/x', ownerToken: 'owner 7' }), + ).test(argvFor(`AgentDeviceRunner.env.${suffix}.xctestrun`)), + true, + ); +}); + +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-xctestrun.test.ts b/packages/platform-apple/src/runner/__tests__/runner-xctestrun.test.ts index 79848916ba..ed1c54f7e6 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 { + buildRunnerSessionXctestrunCleanupPattern, + 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,42 @@ 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-`)); + assert.match( + argv, + new RegExp( + `xcodebuild.*test-without-building.*${buildRunnerSessionXctestrunCleanupPattern({ + deviceId: 'SIM-001', + ownerToken: 'owner-4242-ab12cd34', + })}`, + ), + ); + }); +}); + 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..6572e1348b 100644 --- a/packages/platform-apple/src/runner/runner-artifact-env.ts +++ b/packages/platform-apple/src/runner/runner-artifact-env.ts @@ -6,6 +6,71 @@ 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 knows the owner + * token or does not. Without a token 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 buildRunnerSessionXctestrunCleanupPattern( + params: Readonly<{ deviceId: string; ownerToken?: string | undefined }>, +): string { + const deviceId = escapeForExtendedRegex(sanitizeRunnerSessionNameField(params.deviceId)); + const { ownerToken } = params; + return ownerToken === undefined + ? `${RUNNER_SESSION_XCTESTRUN_NAME_PATTERN}${deviceId}-[0-9]` + : `${RUNNER_SESSION_XCTESTRUN_NAME_PATTERN}${deviceId}-${escapeForExtendedRegex( + sanitizeRunnerSessionNameField(ownerToken), + )}-`; +} + +/** 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 +109,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..ce9ab3f167 100644 --- a/packages/platform-apple/src/runner/runner-disposal.ts +++ b/packages/platform-apple/src/runner/runner-disposal.ts @@ -9,6 +9,7 @@ import { runXcrun, } from './host.ts'; import type { ExecBackgroundResult } from '@agent-device/host-kit/command'; +import { buildRunnerSessionXctestrunCleanupPattern } 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'; @@ -331,9 +332,9 @@ 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]`; + const pattern = `xcodebuild.*test-without-building.*${buildRunnerSessionXctestrunCleanupPattern( + ownerToken === undefined ? { deviceId } : { deviceId, ownerToken }, + )}`; for (const signal of ['TERM', 'KILL'] as const) { try { await runAppleToolCommand('pkill', [`-${signal}`, '-f', pattern], { @@ -353,7 +354,3 @@ async function killRunnerXcodebuildProcesses( } } } - -function escapeRegex(value: string): string { - return value.replaceAll(/[.*+?^${}()|[\]\\]/g, String.raw`\$&`); -} 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`, ]; From f1e27f1cda82d5761743f1fad079afc3848b5d6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 08:39:56 +0200 Subject: [PATCH 2/3] refactor(ios-runner): reclaim a leased launch by the artifact the lease recorded A detached lease rewrites its owner token to `detached-` while the xcodebuild it handed over keeps running under the name the writer gave it, so the token-derived pkill pattern named a file that never existed and the launch survived the reclaim. Lease-backed cleanup now follows the recorded xctestrun basename, which is exactly the argv the launch carries; the device sweep stays for the no-lease case, and the token branch of the old builder is removed. --- .../__tests__/runner-artifact-env.test.ts | 90 ++++++++++++++----- .../runner-lease-claim-takeover.test.ts | 18 +++- .../runner/__tests__/runner-session.test.ts | 52 +++++++++-- .../runner/__tests__/runner-xctestrun.test.ts | 14 +-- .../src/runner/runner-artifact-env.ts | 35 +++++--- .../src/runner/runner-disposal.ts | 19 ++-- .../platform-apple/src/runner/runner-lease.ts | 20 ++++- 7 files changed, 186 insertions(+), 62 deletions(-) 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 index 95956b665e..5038a07fa7 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-artifact-env.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-artifact-env.test.ts @@ -1,7 +1,9 @@ import assert from 'node:assert/strict'; +import path from 'node:path'; import { test } from 'vitest'; import { - buildRunnerSessionXctestrunCleanupPattern, + buildRunnerSessionXctestrunDeviceCleanupPattern, + buildRunnerSessionXctestrunPathCleanupPattern, buildRunnerSessionXctestrunSuffix, } from '../runner-artifact-env.ts'; @@ -10,6 +12,9 @@ import { // 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({ @@ -21,49 +26,55 @@ test('the session suffix keeps the bytes and field order the matchers rely on', ); }); -test('the owner cleanup pattern keeps the bytes released daemons pkilled', () => { - const owned = buildRunnerSessionXctestrunCleanupPattern({ - deviceId: 'SIM-001', - ownerToken: 'owner-4242-ab12cd34', - }); +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-`); assert.equal( - new RegExp(owned).test(argvFor(sessionFileName('SIM-001', 'owner-4242-ab12cd34'))), - true, + 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'))), + 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'))), + 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('a tokenless cleanup 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 owner, so it sweeps by +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 token to be selected. - const tokenless = buildRunnerSessionXctestrunCleanupPattern({ deviceId: 'SIM-002' }); + // spelling; an owner-token name needs its own path to be selected. + const sweep = buildRunnerSessionXctestrunDeviceCleanupPattern('SIM-002'); - assert.equal(tokenless, String.raw`AgentDeviceRunner\.env\.session-SIM-002-[0-9]`); + assert.equal(sweep, String.raw`AgentDeviceRunner\.env\.session-SIM-002-[0-9]`); assert.equal( - new RegExp(tokenless).test(argvFor('AgentDeviceRunner.env.session-SIM-002-8123.xctestrun')), + new RegExp(sweep).test(argvFor('AgentDeviceRunner.env.session-SIM-002-8123.xctestrun')), true, ); assert.equal( - new RegExp(tokenless).test(argvFor(sessionFileName('SIM-002', 'owner-1-ff'))), + 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 name field is sanitized the way the filesystem writer sanitizes it', () => { - // A device id is caller-supplied and the writer flattens it onto disk, so the matcher has to - // flatten it too or it selects a name the launch never had. +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', @@ -71,14 +82,45 @@ test('a name field is sanitized the way the filesystem writer sanitizes it', () }); 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( - buildRunnerSessionXctestrunCleanupPattern({ deviceId: 'SIM 01/x', ownerToken: 'owner 7' }), - ).test(argvFor(`AgentDeviceRunner.env.${suffix}.xctestrun`)), + 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`; } 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..984f33f7e7 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-session.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-session.test.ts @@ -125,6 +125,7 @@ import { validateRunnerDevice, } from '../runner-session.ts'; import { + buildDetachedRunnerLease, cleanupRunnerLeasesForOwner, prepareRunnerLeaseForStartup, runnerOwnerStartTime, @@ -132,6 +133,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 @@ -734,6 +736,41 @@ test('runner session startup reclaims dead foreign runner lease before launching ); }); +test('runner session startup reclaims a detached lease by the artifact that lease recorded', async () => { + // A detached lease keeps the runner's launch name but rewrites its own token to + // `detached-`, so the reclaim has to follow the recorded xctestrun path: a pattern rebuilt + // from the lease's token names a file the launch never carried, and the xcodebuild survives. + const device = { ...IOS_SIMULATOR, id: 'runner-session-detached-lease-sim' }; + const detached = buildDetachedRunnerLease( + makeRunnerLease({ + deviceId: device.id, + ownerToken: 'owner-4242-ab12cd34', + ownerPid: 999_999_999, + ownerStartTime: 'Fri Jun 19 12:01:00 2026', + runnerPid: 999_999_998, + }), + ); + assert.match(detached.ownerToken, /^detached-owner-4242-ab12cd34$/); + mockIsProcessAlive.mockImplementation((pid) => pid !== 999_999_999 && pid !== 999_999_998); + writeRunnerLease(detached); + mockPrepareXctestrunWithEnv.mockResolvedValue({ + xctestrunPath: detached.xctestrunPath, + jsonPath: detached.jsonPath, + }); + const launchArgv = `xcodebuild test-without-building -xctestrun ${detached.xctestrunPath}`; + + const session = await ensureRunnerSession(device, {}); + + assert.equal(session.deviceId, device.id); + const pkillCalls = mockRunAppleToolCommand.mock.calls.filter(isXcodebuildPkillCall); + assert.ok(pkillCalls.length >= 2); + const pattern = String(pkillCalls[0]?.[1]?.[2] ?? ''); + assert.ok( + new RegExp(pattern).test(launchArgv), + `the reclaim pattern ${pattern} must select the launch ${launchArgv}`, + ); +}); + // #1596: lease files outlive their runner (SIGKILLed daemon) and pids get // recycled — the stale-lease cleanup must never signal a pid it cannot prove // is still the leased runner. The recording adapter observes exactly which @@ -741,13 +778,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 +820,12 @@ 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' }, - ]); + assert.equal(xcodebuildCleanups.length, 1); + assert.equal(xcodebuildCleanups[0]?.deviceId, device.id); + assert.equal( + path.basename(String(xcodebuildCleanups[0]?.xctestrunPath)), + 'AgentDeviceRunner.env.session-runner-lease-recycled-pid-sim-owner-dead-recycled-8123.xctestrun', + ); }); 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 ed1c54f7e6..be8a4d2ba6 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-xctestrun.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-xctestrun.test.ts @@ -7,7 +7,7 @@ import path from 'node:path'; import { pathToFileURL } from 'node:url'; import { mkdtempForTestSync } from './tmp-dir.ts'; import { - buildRunnerSessionXctestrunCleanupPattern, + buildRunnerSessionXctestrunPathCleanupPattern, buildRunnerSessionXctestrunSuffix, } from '../runner-artifact-env.ts'; import { appleRunnerTestHost } from '../test-host.ts'; @@ -386,13 +386,17 @@ test('the session xctestrun the writer builds is found by the cleanup matcher', // 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( - `xcodebuild.*test-without-building.*${buildRunnerSessionXctestrunCleanupPattern({ - deviceId: 'SIM-001', - ownerToken: 'owner-4242-ab12cd34', - })}`, + String.raw`xcodebuild.*test-without-building.*AgentDeviceRunner\.env\.session-SIM-001-owner-4242-ab12cd34-8123\.xctestrun`, ), ); }); diff --git a/packages/platform-apple/src/runner/runner-artifact-env.ts b/packages/platform-apple/src/runner/runner-artifact-env.ts index 6572e1348b..b970887f62 100644 --- a/packages/platform-apple/src/runner/runner-artifact-env.ts +++ b/packages/platform-apple/src/runner/runner-artifact-env.ts @@ -46,20 +46,31 @@ export function buildRunnerSessionXctestrunSuffix( } /** - * The `pkill -f` pattern selecting one device's runner launches, for a caller that knows the owner - * token or does not. Without a token the device is followed by the port — the pre-owner-token + * 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 buildRunnerSessionXctestrunCleanupPattern( - params: Readonly<{ deviceId: string; ownerToken?: string | undefined }>, -): string { - const deviceId = escapeForExtendedRegex(sanitizeRunnerSessionNameField(params.deviceId)); - const { ownerToken } = params; - return ownerToken === undefined - ? `${RUNNER_SESSION_XCTESTRUN_NAME_PATTERN}${deviceId}-[0-9]` - : `${RUNNER_SESSION_XCTESTRUN_NAME_PATTERN}${deviceId}-${escapeForExtendedRegex( - sanitizeRunnerSessionNameField(ownerToken), - )}-`; +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. */ diff --git a/packages/platform-apple/src/runner/runner-disposal.ts b/packages/platform-apple/src/runner/runner-disposal.ts index ce9ab3f167..1dc18c2d5e 100644 --- a/packages/platform-apple/src/runner/runner-disposal.ts +++ b/packages/platform-apple/src/runner/runner-disposal.ts @@ -9,7 +9,10 @@ import { runXcrun, } from './host.ts'; import type { ExecBackgroundResult } from '@agent-device/host-kit/command'; -import { buildRunnerSessionXctestrunCleanupPattern } from './runner-artifact-env.ts'; +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'; @@ -20,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'; @@ -328,13 +332,12 @@ async function killRunnerProcessTree( } catch {} } -async function killRunnerXcodebuildProcesses( - deviceId: string, - ownerToken: string | undefined, -): Promise { - const pattern = `xcodebuild.*test-without-building.*${buildRunnerSessionXctestrunCleanupPattern( - ownerToken === undefined ? { deviceId } : { deviceId, ownerToken }, - )}`; +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], { 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); From a11fba324598f4f62671c4b6e0401c3e635db47c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 26 Sep 2026 11:41:12 +0200 Subject: [PATCH 3/3] test(ios-runner): put the reclaim pattern claim in the module that builds it Driving `runnerLeaseCleanupAdapter` directly proves the pkill bytes disposal issues select the launch a lease names, without session-startup choreography. Moves the detached-lease coverage out of the aggregated session suite, which the test-file size ratchet forbids growing. --- .../runner/__tests__/runner-disposal.test.ts | 58 ++++++++++++++++++- .../runner/__tests__/runner-session.test.ts | 44 +------------- 2 files changed, 59 insertions(+), 43 deletions(-) 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-session.test.ts b/packages/platform-apple/src/runner/__tests__/runner-session.test.ts index 984f33f7e7..b7ea9f0c01 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-session.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-session.test.ts @@ -125,7 +125,6 @@ import { validateRunnerDevice, } from '../runner-session.ts'; import { - buildDetachedRunnerLease, cleanupRunnerLeasesForOwner, prepareRunnerLeaseForStartup, runnerOwnerStartTime, @@ -736,41 +735,6 @@ test('runner session startup reclaims dead foreign runner lease before launching ); }); -test('runner session startup reclaims a detached lease by the artifact that lease recorded', async () => { - // A detached lease keeps the runner's launch name but rewrites its own token to - // `detached-`, so the reclaim has to follow the recorded xctestrun path: a pattern rebuilt - // from the lease's token names a file the launch never carried, and the xcodebuild survives. - const device = { ...IOS_SIMULATOR, id: 'runner-session-detached-lease-sim' }; - const detached = buildDetachedRunnerLease( - makeRunnerLease({ - deviceId: device.id, - ownerToken: 'owner-4242-ab12cd34', - ownerPid: 999_999_999, - ownerStartTime: 'Fri Jun 19 12:01:00 2026', - runnerPid: 999_999_998, - }), - ); - assert.match(detached.ownerToken, /^detached-owner-4242-ab12cd34$/); - mockIsProcessAlive.mockImplementation((pid) => pid !== 999_999_999 && pid !== 999_999_998); - writeRunnerLease(detached); - mockPrepareXctestrunWithEnv.mockResolvedValue({ - xctestrunPath: detached.xctestrunPath, - jsonPath: detached.jsonPath, - }); - const launchArgv = `xcodebuild test-without-building -xctestrun ${detached.xctestrunPath}`; - - const session = await ensureRunnerSession(device, {}); - - assert.equal(session.deviceId, device.id); - const pkillCalls = mockRunAppleToolCommand.mock.calls.filter(isXcodebuildPkillCall); - assert.ok(pkillCalls.length >= 2); - const pattern = String(pkillCalls[0]?.[1]?.[2] ?? ''); - assert.ok( - new RegExp(pattern).test(launchArgv), - `the reclaim pattern ${pattern} must select the launch ${launchArgv}`, - ); -}); - // #1596: lease files outlive their runner (SIGKILLed daemon) and pids get // recycled — the stale-lease cleanup must never signal a pid it cannot prove // is still the leased runner. The recording adapter observes exactly which @@ -820,12 +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.equal(xcodebuildCleanups.length, 1); - assert.equal(xcodebuildCleanups[0]?.deviceId, device.id); - assert.equal( - path.basename(String(xcodebuildCleanups[0]?.xctestrunPath)), - 'AgentDeviceRunner.env.session-runner-lease-recycled-pid-sim-owner-dead-recycled-8123.xctestrun', - ); + 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 () => {