diff --git a/src/platforms/apple/core/__tests__/runner-adoption.test.ts b/src/platforms/apple/core/__tests__/runner-adoption.test.ts index 2f30e411e..aed8ef970 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 } 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) => { @@ -27,6 +31,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 +46,8 @@ 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', @@ -82,6 +89,10 @@ beforeEach(() => { mockSendRunnerCommandOnce.mockReset(); mockIsProcessAlive.mockReset(); mockIsProcessAlive.mockReturnValue(false); + mockReadProcessCommand.mockReset(); + mockReadProcessCommand.mockReturnValue(null); + mockReadProcessStartTime.mockReset(); + mockReadProcessStartTime.mockReturnValue('test-process-start'); delete process.env.AGENT_DEVICE_IOS_RUNNER_DETACH; }); @@ -137,6 +148,62 @@ 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 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 + // 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 ec9fcd95a..d09ac6c9b 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,138 @@ 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 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' }; + + 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 b503a0ffb..70a2a93b5 100644 --- a/src/platforms/apple/core/runner/runner-adoption.ts +++ b/src/platforms/apple/core/runner/runner-adoption.ts @@ -10,6 +10,7 @@ import { withRunnerCommandId } from './runner-contract.ts'; import { buildRunnerLease, readStaleRunnerLease, + verifyLeaseRunnerPidIdentity, writeRunnerLease, type RunnerLease, } from './runner-lease.ts'; @@ -69,12 +70,26 @@ 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 — 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); if (!expectedDerived) return skip('expected_derived_unresolved'); if (!lease.xctestrunPath.startsWith(`${expectedDerived}${path.sep}`)) { 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 { diff --git a/src/platforms/apple/core/runner/runner-lease.ts b/src/platforms/apple/core/runner/runner-lease.ts index 69c4779ef..f562dc67a 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,61 @@ async function cleanupLeasedRunnerProcesses( reason, }, }); - await cleanup.cleanupRunnerProcessTree(lease.runnerPid ?? undefined, 'SIGTERM'); + await cleanup.cleanupRunnerProcessTree(resolveVerifiedLeaseRunnerPid(lease), 'SIGTERM'); await cleanup.cleanupRunnerXcodebuildProcesses(lease.deviceId, lease.ownerToken); - await cleanup.cleanupRunnerProcessTree(lease.runnerPid ?? undefined, 'SIGKILL'); + await cleanup.cleanupRunnerProcessTree(resolveVerifiedLeaseRunnerPid(lease), '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. 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; + if (verifyLeaseRunnerPidIdentity(lease, pid)) 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; +} + +/** + * 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'); +} + function buildRunnerOwnerToken(pid: number, startTime: string | null): string { const hash = crypto.createHash('sha256'); hash.update(String(pid));