From 0dc70a37e03c6a480285ab359e3252b4155f1587 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 5 Aug 2026 17:06:20 +0200 Subject: [PATCH 1/3] fix(ios): never signal a recycled runner pid from a stale lease (#1596) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A runner lease file can outlive its runner by days (SIGKILLed daemon), and pids get recycled: cleanupLeasedRunnerProcesses killed lease.runnerPid raw — group kill + direct kill + pkill -P — while only the lease OWNER pid had identity verification. After a pid-space wrap the tree kill lands on whatever process now holds the pid. - record runnerStartTime in the lease at construction (both the fresh-spawn and adoption sites go through buildRunnerLease) - verify the pid before the SIGTERM/SIGKILL tree kills: start-time match, or for legacy leases without one, a runner-shaped xcodebuild command line; otherwise skip the tree kill and emit ios_runner_lease_recycled_pid_skipped - the pattern-based xcodebuild pkill still runs unconditionally, so genuinely stray runner processes are still collected - adoption skips a recycled pid too: the adopted session's disposal would later signal it --- .../core/__tests__/runner-adoption.test.ts | 11 ++ .../core/__tests__/runner-session.test.ts | 118 ++++++++++++++++++ .../apple/core/runner/runner-adoption.ts | 7 +- .../apple/core/runner/runner-lease.ts | 48 ++++++- 4 files changed, 180 insertions(+), 4 deletions(-) diff --git a/src/platforms/apple/core/__tests__/runner-adoption.test.ts b/src/platforms/apple/core/__tests__/runner-adoption.test.ts index 2f30e411e7..c2737f84fa 100644 --- a/src/platforms/apple/core/__tests__/runner-adoption.test.ts +++ b/src/platforms/apple/core/__tests__/runner-adoption.test.ts @@ -137,6 +137,17 @@ test('adoption succeeds for a live, matching, probe-healthy runner', async () => expect(readStaleRunnerLease(simulator.id)).toBeNull(); }); +test('adoption is skipped for a recycled runner pid (start time mismatch)', async () => { + // The live process on the leased pid started at a different time than the + // lease recorded — pid recycled since the owner died (#1596). Adopting it + // would make later disposal signal an innocent process. + writeStaleLease({ runnerStartTime: 'runner-original-start' }); + mockIsProcessAlive.mockReturnValue(true); + + expect(await tryAdoptRunnerSessionFromLease(simulator, {})).toBeNull(); + expect(mockSendRunnerCommandOnce).not.toHaveBeenCalled(); +}); + test('adoption is skipped for devices in a custom simulator set', async () => { writeStaleLease(); mockIsProcessAlive.mockReturnValue(true); diff --git a/src/platforms/apple/core/__tests__/runner-session.test.ts b/src/platforms/apple/core/__tests__/runner-session.test.ts index ec9fcd95a0..8203e71b9b 100644 --- a/src/platforms/apple/core/__tests__/runner-session.test.ts +++ b/src/platforms/apple/core/__tests__/runner-session.test.ts @@ -24,6 +24,7 @@ const { mockIsProcessAlive, mockIsProcessGroupAlive, mockPrepareXctestrunWithEnv, + mockReadProcessCommand, mockReadProcessStartTime, mockResolveExpectedRunnerCacheMetadata, mockResolveRunnerDerivedPath, @@ -46,6 +47,7 @@ const { // shells out to `ps` with a 1s timeout that can miss under CPU contention, // flipping a live owner to 'owner-process-dead'. Deterministic value, no // shell-out; identity is still enforced by pid in beforeEach below. + mockReadProcessCommand: vi.fn((_pid: number) => null as string | null), mockReadProcessStartTime: vi.fn((_pid: number) => 'fixed-test-owner-start-time' as string | null), mockResolveExpectedRunnerCacheMetadata: vi.fn(), mockResolveRunnerDerivedPath: vi.fn(), @@ -75,6 +77,7 @@ vi.mock('../../../../utils/host-process.ts', async () => { ...actual, isProcessAlive: mockIsProcessAlive, isProcessGroupAlive: mockIsProcessGroupAlive, + readProcessCommand: mockReadProcessCommand, readProcessStartTime: mockReadProcessStartTime, }; }); @@ -129,11 +132,13 @@ import { } from '../runner/runner-session.ts'; import { cleanupRunnerLeasesForOwner, + prepareRunnerLeaseForStartup, RUNNER_OWNER_START_TIME, RUNNER_OWNER_TOKEN, setRunnerLeaseOwnerStateDir, writeRunnerLease, type RunnerLease, + type RunnerLeaseCleanupAdapter, } from '../runner/runner-lease.ts'; beforeEach(async () => { @@ -164,6 +169,7 @@ beforeEach(async () => { mockRunAppleToolCommand.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' }); mockIsProcessAlive.mockReturnValue(true); mockIsProcessGroupAlive.mockReturnValue(false); + mockReadProcessCommand.mockReturnValue(null); // Our pid reads back its fixed start time; any other pid reads as // not-found, same as a real `ps` miss. Dead-lease tests use fabricated // pids already rejected by mockIsProcessAlive before this is consulted. @@ -1113,6 +1119,118 @@ test('runner session startup reclaims dead foreign runner lease before launching ); }); +// #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 +// pid the cleanup would kill; the pattern-based xcodebuild pkill is a +// 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 adapter: RunnerLeaseCleanupAdapter = { + async cleanupRunnerProcessTree(pid, signal) { + treeKills.push({ pid, signal }); + }, + async cleanupRunnerXcodebuildProcesses(deviceId, ownerToken) { + xcodebuildCleanups.push({ deviceId, ownerToken }); + }, + cleanupTempFile() {}, + }; + return { adapter, treeKills, xcodebuildCleanups }; +} + +function writeStaleLeaseWithRunner( + deviceId: string, + runner: Pick, +): void { + mockIsProcessAlive.mockImplementation((pid) => pid !== 999_999_999); + writeRunnerLease( + makeRunnerLease({ + deviceId, + ownerToken: 'owner-dead-recycled', + ownerPid: 999_999_999, + ...runner, + }), + ); +} + +test('stale-lease cleanup does not signal a recycled runner pid (start time mismatch)', async () => { + const device = { ...IOS_SIMULATOR, id: 'runner-lease-recycled-pid-sim' }; + writeStaleLeaseWithRunner(device.id, { runnerPid: 55_555, runnerStartTime: 'lease-time-start' }); + mockReadProcessStartTime.mockImplementation((pid: number) => + pid === 55_555 ? 'different-newer-start' : null, + ); + const { adapter, treeKills, xcodebuildCleanups } = makeRecordingCleanupAdapter(); + + await prepareRunnerLeaseForStartup(device.id, adapter); + + assert.deepEqual(treeKills, [ + { pid: undefined, signal: 'SIGTERM' }, + { pid: undefined, signal: 'SIGKILL' }, + ]); + assert.deepEqual(xcodebuildCleanups, [ + { deviceId: device.id, ownerToken: 'owner-dead-recycled' }, + ]); +}); + +test('stale-lease cleanup signals the runner pid when its start time still matches', async () => { + const device = { ...IOS_SIMULATOR, id: 'runner-lease-verified-pid-sim' }; + writeStaleLeaseWithRunner(device.id, { runnerPid: 55_555, runnerStartTime: 'lease-time-start' }); + mockReadProcessStartTime.mockImplementation((pid: number) => + pid === 55_555 ? 'lease-time-start' : null, + ); + const { adapter, treeKills } = makeRecordingCleanupAdapter(); + + await prepareRunnerLeaseForStartup(device.id, adapter); + + assert.deepEqual(treeKills, [ + { pid: 55_555, signal: 'SIGTERM' }, + { pid: 55_555, signal: 'SIGKILL' }, + ]); +}); + +test('stale-lease cleanup without a recorded start time trusts only runner-shaped commands', async () => { + const device = { ...IOS_SIMULATOR, id: 'runner-lease-legacy-pid-sim' }; + + writeStaleLeaseWithRunner(device.id, { runnerPid: 55_555, runnerStartTime: null }); + mockReadProcessCommand.mockImplementation((pid: number) => + pid === 55_555 ? 'node /usr/local/bin/opencode run --model gpt-high' : null, + ); + const foreign = makeRecordingCleanupAdapter(); + await prepareRunnerLeaseForStartup(device.id, foreign.adapter); + assert.deepEqual(foreign.treeKills, [ + { pid: undefined, signal: 'SIGTERM' }, + { pid: undefined, signal: 'SIGKILL' }, + ]); + + writeStaleLeaseWithRunner(device.id, { runnerPid: 55_555, runnerStartTime: null }); + mockReadProcessCommand.mockImplementation((pid: number) => + pid === 55_555 + ? 'xcodebuild test-without-building -xctestrun /tmp/AgentDeviceRunner.env.session-x.xctestrun' + : null, + ); + const runner = makeRecordingCleanupAdapter(); + await prepareRunnerLeaseForStartup(device.id, runner.adapter); + assert.deepEqual(runner.treeKills, [ + { pid: 55_555, signal: 'SIGTERM' }, + { pid: 55_555, signal: 'SIGKILL' }, + ]); +}); + +test('stale-lease cleanup skips a dead runner pid entirely', async () => { + const device = { ...IOS_SIMULATOR, id: 'runner-lease-dead-pid-sim' }; + writeStaleLeaseWithRunner(device.id, { runnerPid: 55_555, runnerStartTime: 'lease-time-start' }); + mockIsProcessAlive.mockImplementation((pid) => pid !== 999_999_999 && pid !== 55_555); + const { adapter, treeKills } = makeRecordingCleanupAdapter(); + + await prepareRunnerLeaseForStartup(device.id, adapter); + + assert.deepEqual(treeKills, [ + { pid: undefined, signal: 'SIGTERM' }, + { pid: undefined, signal: 'SIGKILL' }, + ]); +}); + test('runner session startup reclaims a foreign runner lease whose owner state dir is gone', async () => { const device = { ...IOS_SIMULATOR, id: 'runner-session-owner-state-dir-gone-sim' }; const goneStateDir = mkdtempForTestSync('agent-device-owner-state-dir-gone-'); diff --git a/src/platforms/apple/core/runner/runner-adoption.ts b/src/platforms/apple/core/runner/runner-adoption.ts index b503a0ffb5..1e3abd585b 100644 --- a/src/platforms/apple/core/runner/runner-adoption.ts +++ b/src/platforms/apple/core/runner/runner-adoption.ts @@ -1,7 +1,7 @@ import path from 'node:path'; import { resolveIosSimulatorDeviceSetPath } from '../../../../utils/device-isolation.ts'; import { emitDiagnostic } from '../../../../utils/diagnostics.ts'; -import { isProcessAlive } from '../../../../utils/host-process.ts'; +import { isProcessAlive, readProcessStartTime } from '../../../../utils/host-process.ts'; import { parseBooleanLiteral } from '../../../../utils/source-value.ts'; import type { DeviceInfo } from '@agent-device/kernel/device'; import type { ExecResult } from '../../../../utils/exec.ts'; @@ -69,6 +69,11 @@ export async function tryAdoptRunnerSessionFromLease( const runnerPid = lease.runnerPid; if (!runnerPid) return skip('runner_pid_missing'); if (!isProcessAlive(runnerPid)) return skip('runner_process_dead'); + // The adopted session later signals this pid on disposal, so a recycled pid + // must never be adopted even if some process answers the leased port. + if (lease.runnerStartTime && readProcessStartTime(runnerPid) !== lease.runnerStartTime) { + return skip('runner_pid_recycled'); + } const expectedDerived = resolveExpectedDerivedPath(device); if (!expectedDerived) return skip('expected_derived_unresolved'); if (!lease.xctestrunPath.startsWith(`${expectedDerived}${path.sep}`)) { diff --git a/src/platforms/apple/core/runner/runner-lease.ts b/src/platforms/apple/core/runner/runner-lease.ts index 69c4779ef6..fdaeb379c6 100644 --- a/src/platforms/apple/core/runner/runner-lease.ts +++ b/src/platforms/apple/core/runner/runner-lease.ts @@ -5,7 +5,11 @@ import path from 'node:path'; import { emitDiagnostic } from '../../../../utils/diagnostics.ts'; import { AppError } from '@agent-device/kernel/errors'; import { acquireProcessLock } from '../../../../utils/process-lock.ts'; -import { readProcessStartTime } from '../../../../utils/host-process.ts'; +import { + isProcessAlive, + readProcessCommand, + readProcessStartTime, +} from '../../../../utils/host-process.ts'; import { classifyOwnerLiveness } from '../../../../utils/owner-identity.ts'; import type { RunnerLogicalLeaseContext } from '@agent-device/contracts/platform'; @@ -29,6 +33,7 @@ export type RunnerLease = { ownerStateDir?: string; sessionId: string; runnerPid: number | null; + runnerStartTime?: string | null; port: number; xctestrunPath: string; jsonPath: string; @@ -76,6 +81,7 @@ export function buildRunnerLease(params: { ownerStateDir: readCurrentStateDir(), sessionId: params.sessionId, runnerPid: params.runnerPid ?? null, + runnerStartTime: params.runnerPid ? readProcessStartTime(params.runnerPid) : null, port: params.port, xctestrunPath: params.xctestrunPath, jsonPath: params.jsonPath, @@ -372,6 +378,7 @@ function normalizeRunnerLease(value: unknown, deviceId: string): RunnerLease | n ownerStartTime: readOptionalString(raw.ownerStartTime), ownerStateDir: readOptionalString(raw.ownerStateDir) ?? undefined, runnerPid: readPositiveInteger(raw.runnerPid), + runnerStartTime: readOptionalString(raw.runnerStartTime), }; } @@ -455,14 +462,49 @@ async function cleanupLeasedRunnerProcesses( reason, }, }); - await cleanup.cleanupRunnerProcessTree(lease.runnerPid ?? undefined, 'SIGTERM'); + const runnerPid = resolveVerifiedLeaseRunnerPid(lease); + await cleanup.cleanupRunnerProcessTree(runnerPid, 'SIGTERM'); await cleanup.cleanupRunnerXcodebuildProcesses(lease.deviceId, lease.ownerToken); - await cleanup.cleanupRunnerProcessTree(lease.runnerPid ?? undefined, 'SIGKILL'); + await cleanup.cleanupRunnerProcessTree(runnerPid, 'SIGKILL'); cleanup.cleanupTempFile(lease.xctestrunPath); cleanup.cleanupTempFile(lease.jsonPath); releaseRunnerLease(lease); } +/** + * A lease file can outlive its runner by days (SIGKILLed daemon), and pids are + * recycled — killing `lease.runnerPid` raw would signal whatever process now + * holds that pid, including its whole process group (#1596). Only return the + * pid when the live process is provably still the leased runner: matching + * recorded start time, or — for leases written before `runnerStartTime` + * existed — a command line that looks like the runner's xcodebuild. The + * pattern-based xcodebuild pkill in the cleanup adapter is unaffected and + * still collects genuinely stray runner processes. + */ +function resolveVerifiedLeaseRunnerPid(lease: RunnerLease): number | undefined { + const pid = lease.runnerPid ?? undefined; + if (!pid || !isProcessAlive(pid)) return undefined; + const verified = lease.runnerStartTime + ? readProcessStartTime(pid) === lease.runnerStartTime + : isRunnerXcodebuildCommand(readProcessCommand(pid)); + if (verified) return pid; + emitDiagnostic({ + level: 'warn', + phase: 'ios_runner_lease_recycled_pid_skipped', + data: { + deviceId: lease.deviceId, + runnerPid: pid, + runnerStartTime: lease.runnerStartTime ?? null, + sessionId: lease.sessionId, + }, + }); + return undefined; +} + +function isRunnerXcodebuildCommand(command: string | null): boolean { + return !!command && command.includes('xcodebuild') && command.includes('AgentDeviceRunner'); +} + function buildRunnerOwnerToken(pid: number, startTime: string | null): string { const hash = crypto.createHash('sha256'); hash.update(String(pid)); From 3e128b2fe48679fee0c8162f23786817a768a1c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 5 Aug 2026 18:17:20 +0200 Subject: [PATCH 2/3] fix: re-verify lease pid identity before each signal and during legacy adoption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stale-lease cleanup verified the runner pid once and reused it for both SIGTERM and SIGKILL; the runner usually dies on the SIGTERM, and the pid can be recycled while the awaited xcodebuild sweep runs before the escalation. Each signal now resolves a freshly verified pid. Adoption skipped identity verification entirely for legacy leases without a recorded runnerStartTime, then re-stamped the lease with the live pid's start time — laundering a recycled pid into a strongly verified lease that disposal would later kill. Adoption now shares the disposal path's verification, including the runner-shaped command-line fallback for legacy leases. --- .../core/__tests__/runner-adoption.test.ts | 34 ++++++++++++++++++- .../core/__tests__/runner-session.test.ts | 20 +++++++++++ .../apple/core/runner/runner-adoption.ts | 12 ++++--- .../apple/core/runner/runner-lease.ts | 32 +++++++++++------ 4 files changed, 83 insertions(+), 15 deletions(-) diff --git a/src/platforms/apple/core/__tests__/runner-adoption.test.ts b/src/platforms/apple/core/__tests__/runner-adoption.test.ts index c2737f84fa..c7e7efdbf3 100644 --- a/src/platforms/apple/core/__tests__/runner-adoption.test.ts +++ b/src/platforms/apple/core/__tests__/runner-adoption.test.ts @@ -15,7 +15,7 @@ import { tryAdoptRunnerSessionFromLease, } from '../runner/runner-adoption.ts'; import { sendRunnerCommandOnce } from '../runner/runner-transport.ts'; -import { isProcessAlive } from '../../../../utils/host-process.ts'; +import { isProcessAlive, readProcessCommand } from '../../../../utils/host-process.ts'; import { mkdtempForTestSync } from '../../../../__tests__/test-utils/tmp-dir.ts'; vi.mock('../runner/runner-transport.ts', async (importOriginal) => { @@ -27,6 +27,7 @@ vi.mock('../../../../utils/host-process.ts', async (importOriginal) => { return { ...actual, isProcessAlive: vi.fn(() => false), + readProcessCommand: vi.fn(() => null), readProcessStartTime: vi.fn(() => 'test-process-start'), }; }); @@ -41,6 +42,7 @@ vi.mock('../runner/runner-xctestrun.ts', async (importOriginal) => { const mockSendRunnerCommandOnce = vi.mocked(sendRunnerCommandOnce); const mockIsProcessAlive = vi.mocked(isProcessAlive); +const mockReadProcessCommand = vi.mocked(readProcessCommand); const simulator: DeviceInfo = { platform: 'apple', @@ -82,6 +84,8 @@ beforeEach(() => { mockSendRunnerCommandOnce.mockReset(); mockIsProcessAlive.mockReset(); mockIsProcessAlive.mockReturnValue(false); + mockReadProcessCommand.mockReset(); + mockReadProcessCommand.mockReturnValue(null); delete process.env.AGENT_DEVICE_IOS_RUNNER_DETACH; }); @@ -148,6 +152,34 @@ test('adoption is skipped for a recycled runner pid (start time mismatch)', asyn expect(mockSendRunnerCommandOnce).not.toHaveBeenCalled(); }); +test('adoption is skipped for a legacy lease whose live pid is not runner-shaped', async () => { + // Leases written before `runnerStartTime` existed carry no start time; the + // only identity evidence left is the command line. An unverified pid must + // not be adopted — adoption re-stamps the lease with the live pid's start + // time, so a recycled pid would be laundered into a strongly-verified lease + // that disposal later kills. + writeStaleLease({ runnerStartTime: null }); + mockIsProcessAlive.mockReturnValue(true); + mockReadProcessCommand.mockReturnValue('node /usr/local/bin/opencode run --model gpt-high'); + + expect(await tryAdoptRunnerSessionFromLease(simulator, {})).toBeNull(); + expect(mockSendRunnerCommandOnce).not.toHaveBeenCalled(); +}); + +test('adoption accepts a legacy lease whose live pid is runner-shaped', async () => { + writeStaleLease({ runnerStartTime: null }); + mockIsProcessAlive.mockReturnValue(true); + mockReadProcessCommand.mockReturnValue( + 'xcodebuild test-without-building -xctestrun /tmp/AgentDeviceRunner.env.session-x.xctestrun', + ); + mockSendRunnerCommandOnce.mockResolvedValue(new Response(JSON.stringify({ ok: true }))); + + const session = await tryAdoptRunnerSessionFromLease(simulator, {}); + + expect(session?.ready).toBe(true); + expect(session?.child.pid).toBe(424242); +}); + test('adoption is skipped for devices in a custom simulator set', async () => { writeStaleLease(); mockIsProcessAlive.mockReturnValue(true); diff --git a/src/platforms/apple/core/__tests__/runner-session.test.ts b/src/platforms/apple/core/__tests__/runner-session.test.ts index 8203e71b9b..d09ac6c9bd 100644 --- a/src/platforms/apple/core/__tests__/runner-session.test.ts +++ b/src/platforms/apple/core/__tests__/runner-session.test.ts @@ -1189,6 +1189,26 @@ test('stale-lease cleanup signals the runner pid when its start time still match ]); }); +test('stale-lease cleanup re-verifies the pid before the SIGKILL escalation', async () => { + const device = { ...IOS_SIMULATOR, id: 'runner-lease-recycled-between-signals-sim' }; + writeStaleLeaseWithRunner(device.id, { runnerPid: 55_555, runnerStartTime: 'lease-time-start' }); + // The runner dies on SIGTERM and its pid is recycled while the awaited + // xcodebuild sweep runs — the SIGKILL escalation must not trust the + // verification performed for SIGTERM. + let verifications = 0; + mockReadProcessStartTime.mockImplementation((pid: number) => + pid === 55_555 ? (++verifications === 1 ? 'lease-time-start' : 'recycled-newer-start') : null, + ); + const { adapter, treeKills } = makeRecordingCleanupAdapter(); + + await prepareRunnerLeaseForStartup(device.id, adapter); + + assert.deepEqual(treeKills, [ + { pid: 55_555, signal: 'SIGTERM' }, + { pid: undefined, signal: 'SIGKILL' }, + ]); +}); + test('stale-lease cleanup without a recorded start time trusts only runner-shaped commands', async () => { const device = { ...IOS_SIMULATOR, id: 'runner-lease-legacy-pid-sim' }; diff --git a/src/platforms/apple/core/runner/runner-adoption.ts b/src/platforms/apple/core/runner/runner-adoption.ts index 1e3abd585b..59af958aa6 100644 --- a/src/platforms/apple/core/runner/runner-adoption.ts +++ b/src/platforms/apple/core/runner/runner-adoption.ts @@ -1,7 +1,7 @@ import path from 'node:path'; import { resolveIosSimulatorDeviceSetPath } from '../../../../utils/device-isolation.ts'; import { emitDiagnostic } from '../../../../utils/diagnostics.ts'; -import { isProcessAlive, readProcessStartTime } from '../../../../utils/host-process.ts'; +import { isProcessAlive } from '../../../../utils/host-process.ts'; import { parseBooleanLiteral } from '../../../../utils/source-value.ts'; import type { DeviceInfo } from '@agent-device/kernel/device'; import type { ExecResult } from '../../../../utils/exec.ts'; @@ -10,6 +10,7 @@ import { withRunnerCommandId } from './runner-contract.ts'; import { buildRunnerLease, readStaleRunnerLease, + verifyLeaseRunnerPidIdentity, writeRunnerLease, type RunnerLease, } from './runner-lease.ts'; @@ -69,9 +70,12 @@ export async function tryAdoptRunnerSessionFromLease( const runnerPid = lease.runnerPid; if (!runnerPid) return skip('runner_pid_missing'); if (!isProcessAlive(runnerPid)) return skip('runner_process_dead'); - // The adopted session later signals this pid on disposal, so a recycled pid - // must never be adopted even if some process answers the leased port. - if (lease.runnerStartTime && readProcessStartTime(runnerPid) !== lease.runnerStartTime) { + // The adopted session later signals this pid on disposal — and adoption + // re-stamps the lease with the live pid's start time — so a pid that cannot + // be proven to still be the leased runner must never be adopted, even if + // some process answers the leased port. Legacy leases without a recorded + // start time fall back to the runner-shaped command-line check. + if (!verifyLeaseRunnerPidIdentity(lease, runnerPid)) { return skip('runner_pid_recycled'); } const expectedDerived = resolveExpectedDerivedPath(device); diff --git a/src/platforms/apple/core/runner/runner-lease.ts b/src/platforms/apple/core/runner/runner-lease.ts index fdaeb379c6..f562dc67a4 100644 --- a/src/platforms/apple/core/runner/runner-lease.ts +++ b/src/platforms/apple/core/runner/runner-lease.ts @@ -462,10 +462,9 @@ async function cleanupLeasedRunnerProcesses( reason, }, }); - const runnerPid = resolveVerifiedLeaseRunnerPid(lease); - await cleanup.cleanupRunnerProcessTree(runnerPid, 'SIGTERM'); + await cleanup.cleanupRunnerProcessTree(resolveVerifiedLeaseRunnerPid(lease), 'SIGTERM'); await cleanup.cleanupRunnerXcodebuildProcesses(lease.deviceId, lease.ownerToken); - await cleanup.cleanupRunnerProcessTree(runnerPid, 'SIGKILL'); + await cleanup.cleanupRunnerProcessTree(resolveVerifiedLeaseRunnerPid(lease), 'SIGKILL'); cleanup.cleanupTempFile(lease.xctestrunPath); cleanup.cleanupTempFile(lease.jsonPath); releaseRunnerLease(lease); @@ -475,19 +474,17 @@ async function cleanupLeasedRunnerProcesses( * A lease file can outlive its runner by days (SIGKILLed daemon), and pids are * recycled — killing `lease.runnerPid` raw would signal whatever process now * holds that pid, including its whole process group (#1596). Only return the - * pid when the live process is provably still the leased runner: matching - * recorded start time, or — for leases written before `runnerStartTime` - * existed — a command line that looks like the runner's xcodebuild. The + * pid when the live process is provably still the leased runner. Callers must + * resolve immediately before each signal and never reuse a resolved pid across + * an await: the runner usually dies on SIGTERM, and its pid can be recycled + * while the awaited xcodebuild sweep runs before the SIGKILL escalation. The * pattern-based xcodebuild pkill in the cleanup adapter is unaffected and * still collects genuinely stray runner processes. */ function resolveVerifiedLeaseRunnerPid(lease: RunnerLease): number | undefined { const pid = lease.runnerPid ?? undefined; if (!pid || !isProcessAlive(pid)) return undefined; - const verified = lease.runnerStartTime - ? readProcessStartTime(pid) === lease.runnerStartTime - : isRunnerXcodebuildCommand(readProcessCommand(pid)); - if (verified) return pid; + if (verifyLeaseRunnerPidIdentity(lease, pid)) return pid; emitDiagnostic({ level: 'warn', phase: 'ios_runner_lease_recycled_pid_skipped', @@ -501,6 +498,21 @@ function resolveVerifiedLeaseRunnerPid(lease: RunnerLease): number | undefined { return undefined; } +/** + * Is the live process holding `pid` provably still the runner this lease + * recorded? Matching recorded start time, or — for leases written before + * `runnerStartTime` existed — a command line that looks like the runner's + * xcodebuild. Shared with adoption: an unverified pid must never be adopted, + * because adoption re-stamps the lease with the live pid's start time and + * would launder a recycled pid into a strongly-verified lease that disposal + * later kills (#1596). + */ +export function verifyLeaseRunnerPidIdentity(lease: RunnerLease, pid: number): boolean { + return lease.runnerStartTime + ? readProcessStartTime(pid) === lease.runnerStartTime + : isRunnerXcodebuildCommand(readProcessCommand(pid)); +} + function isRunnerXcodebuildCommand(command: string | null): boolean { return !!command && command.includes('xcodebuild') && command.includes('AgentDeviceRunner'); } From 2ef6c0c7d3cbbfb85bf802718df3733ba536a674 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 5 Aug 2026 18:28:57 +0200 Subject: [PATCH 3/3] fix: re-verify pid identity after the awaited adoption probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The uptime probe is the last await before the adopted lease re-stamps the pid with its live start time; the xcodebuild can exit and its pid be recycled during that network round-trip while the old port still answers. Re-verify after the probe — everything from there to the lease write is synchronous. --- .../core/__tests__/runner-adoption.test.ts | 26 ++++++++++++++++++- .../apple/core/runner/runner-adoption.ts | 6 +++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/platforms/apple/core/__tests__/runner-adoption.test.ts b/src/platforms/apple/core/__tests__/runner-adoption.test.ts index c7e7efdbf3..aed8ef9702 100644 --- a/src/platforms/apple/core/__tests__/runner-adoption.test.ts +++ b/src/platforms/apple/core/__tests__/runner-adoption.test.ts @@ -15,7 +15,11 @@ import { tryAdoptRunnerSessionFromLease, } from '../runner/runner-adoption.ts'; import { sendRunnerCommandOnce } from '../runner/runner-transport.ts'; -import { isProcessAlive, readProcessCommand } from '../../../../utils/host-process.ts'; +import { + isProcessAlive, + readProcessCommand, + readProcessStartTime, +} from '../../../../utils/host-process.ts'; import { mkdtempForTestSync } from '../../../../__tests__/test-utils/tmp-dir.ts'; vi.mock('../runner/runner-transport.ts', async (importOriginal) => { @@ -43,6 +47,7 @@ vi.mock('../runner/runner-xctestrun.ts', async (importOriginal) => { const mockSendRunnerCommandOnce = vi.mocked(sendRunnerCommandOnce); const mockIsProcessAlive = vi.mocked(isProcessAlive); const mockReadProcessCommand = vi.mocked(readProcessCommand); +const mockReadProcessStartTime = vi.mocked(readProcessStartTime); const simulator: DeviceInfo = { platform: 'apple', @@ -86,6 +91,8 @@ beforeEach(() => { mockIsProcessAlive.mockReturnValue(false); mockReadProcessCommand.mockReset(); mockReadProcessCommand.mockReturnValue(null); + mockReadProcessStartTime.mockReset(); + mockReadProcessStartTime.mockReturnValue('test-process-start'); delete process.env.AGENT_DEVICE_IOS_RUNNER_DETACH; }); @@ -152,6 +159,23 @@ test('adoption is skipped for a recycled runner pid (start time mismatch)', asyn expect(mockSendRunnerCommandOnce).not.toHaveBeenCalled(); }); +test('adoption is refused when the pid is recycled while the uptime probe is in flight', async () => { + // Identity holds at the first check, then the xcodebuild exits and its pid + // is recycled during the awaited probe while the old port still answers. + // Adoption must re-verify after the probe: re-stamping the recycled pid + // would hand disposal a strongly-verified lease over an innocent process. + const lease = writeStaleLease(); + mockIsProcessAlive.mockReturnValue(true); + mockSendRunnerCommandOnce.mockImplementation(async () => { + mockReadProcessStartTime.mockReturnValue('recycled-during-probe-start'); + return new Response(JSON.stringify({ ok: true })); + }); + + expect(await tryAdoptRunnerSessionFromLease(simulator, {})).toBeNull(); + // The stale lease must survive untouched — ownership was never transferred. + expect(readStaleRunnerLease(simulator.id)?.ownerToken).toBe(lease.ownerToken); +}); + test('adoption is skipped for a legacy lease whose live pid is not runner-shaped', async () => { // Leases written before `runnerStartTime` existed carry no start time; the // only identity evidence left is the command line. An unverified pid must diff --git a/src/platforms/apple/core/runner/runner-adoption.ts b/src/platforms/apple/core/runner/runner-adoption.ts index 59af958aa6..70a2a93b51 100644 --- a/src/platforms/apple/core/runner/runner-adoption.ts +++ b/src/platforms/apple/core/runner/runner-adoption.ts @@ -84,6 +84,12 @@ export async function tryAdoptRunnerSessionFromLease( return skip('artifact_fingerprint_mismatch'); } if (!(await probeRunnerAnswersUptime(device, lease.port))) return skip('probe_failed'); + // The probe awaited network I/O — the xcodebuild can have exited and its pid + // been recycled while the old port still answers. Re-verify before the + // adopted lease re-stamps the pid; everything below is synchronous. + if (!isProcessAlive(runnerPid) || !verifyLeaseRunnerPidIdentity(lease, runnerPid)) { + return skip('runner_pid_recycled'); + } const session = buildAdoptedRunnerSession(device, lease, runnerPid, expectedDerived, options); try {