fix(ios): never signal a recycled runner pid from a stale lease

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);

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<RunnerLease, 'runnerPid' | 'runnerStartTime'>,
+): 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-');

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 {