fix(apple-runner): a redirect's best-effort give-back forgives only its own release

The swallow written last round was too wide. `release()` does two things: it restores the host's own
`~/Library/Developer/XCTestDevices` from the backup, and it gives the device-set lock back. The
second is the one whose failure a teardown can afford, because the claim is spent and a reclaim from
this process now reads it as dead. The first is a fact about this machine — without it the symlink
stays pointed at the agent-device set and every later `simctl` run sees the wrong devices — and it
was going into the same `catch {}` that runner-session and runner-disposal already had.

Both sites now call one handle method, `releaseBestEffort`, which drops only an AppError carrying
`ownerReleaseUnverified` and rethrows anything else; `release` stays strict for the build path, where
`withProcessLock` owns which of two failures gets reported. A test where the restore's `renameSync`
answers EACCES pins the rethrow, and the mutation is the bare `catch {}` returning. The standalone
best-effort helper is gone, so there is one place that decides what a teardown forgives.

Author: thymikee

packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts

@@ -1,13 +1,12 @@
 import assert from 'node:assert/strict';
 import fs from 'node:fs';
 import path from 'node:path';
-import { test } from 'vitest';
+import { test, vi } from 'vitest';
 import { AppError } from '@agent-device/kernel/errors';
 import type { DeviceInfo } from '@agent-device/kernel/device';
 import { mkdtempForTestSync } from './tmp-dir.ts';
 import {
   acquireXcodebuildSimulatorSetRedirect,
-  releaseXcodebuildSimulatorSetRedirectBestEffort,
   withXcodebuildSimulatorSetRedirect,
 } from '../runner-device-set.ts';
 
@@ -127,7 +126,48 @@ test('a redirect handed back after its task keeps quiet about a release it canno
     assert.notEqual(redirect, null);
     makeReleaseUnverifiable(paths);
 
-    await releaseXcodebuildSimulatorSetRedirectBestEffort(redirect);
+    await redirect?.releaseBestEffort();
     assert.equal(fs.existsSync(paths.lockDirPath), true);
   });
 });
+
+test('a redirect that could not restore the host device set reports it instead of swallowing it', async () => {
+  await withTempDir('device-set-restore-failure-', async (root) => {
+    const paths = makeRedirectPaths(root);
+    fs.mkdirSync(paths.requestedSetPath, { recursive: true });
+    // The host has a device set of its own, so giving the redirect back renames it out of the
+    // backup. Without this the release has nothing to restore and no rename to attempt.
+    fs.mkdirSync(paths.xctestDeviceSetPath, { recursive: true });
+    fs.writeFileSync(path.join(paths.xctestDeviceSetPath, 'host-device.txt'), 'the host owns this');
+    const redirect = await acquireXcodebuildSimulatorSetRedirect(makeScopedSimulator(paths), {
+      lockDirPath: paths.lockDirPath,
+      xctestDeviceSetPath: paths.xctestDeviceSetPath,
+    });
+    assert.notEqual(redirect, null);
+
+    // The restore of the host's own `XCTestDevices` is a rename back from the backup, and a
+    // refusal there is a fact about this machine that no caller may lose.
+    let attempted = false;
+    const realRename = fs.renameSync;
+    const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation(((
+      from: fs.PathLike,
+      to: fs.PathLike,
+    ) => {
+      if (String(to) === paths.xctestDeviceSetPath && !attempted) {
+        attempted = true;
+        throw Object.assign(new Error('EACCES: permission denied'), { code: 'EACCES' });
+      }
+      return realRename(from as string, to as string);
+    }) as typeof fs.renameSync);
+
+    try {
+      await assert.rejects(
+        () => redirect!.releaseBestEffort(),
+        (error: unknown) => (error as NodeJS.ErrnoException).code === 'EACCES',
+      );
+      assert.equal(attempted, true);
+    } finally {
+      renameSpy.mockRestore();
+    }
+  });
+});

packages/platform-apple/src/runner/__tests__/runner-request-cancellation.test.ts

@@ -157,6 +157,7 @@ beforeEach(async () => {
   mockResolveRunnerDerivedPath.mockReturnValue('/tmp/derived');
   mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue({
     release: mockRedirectRelease,
+    releaseBestEffort: mockRedirectRelease,
   });
   mockRunCmdBackground.mockReturnValue(makeBackgroundRunner(4242));
   mockRunAppleToolCommand.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' });

packages/platform-apple/src/runner/__tests__/runner-session-speculative.test.ts

@@ -138,7 +138,10 @@ beforeEach(async () => {
   });
   mockResolveExpectedRunnerCacheMetadata.mockReturnValue({ schemaVersion: 1 });
   mockResolveRunnerDerivedPath.mockReturnValue('/tmp/derived');
-  mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue({ release: mockRedirectRelease });
+  mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue({
+    release: mockRedirectRelease,
+    releaseBestEffort: mockRedirectRelease,
+  });
   mockRunCmdBackground.mockReturnValue(makeBackgroundRunner(4242));
   mockRunAppleToolCommand.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' });
   mockIsProcessAlive.mockReturnValue(true);
@@ -189,7 +192,7 @@ test('a release that arrives while the speculative start is still in flight stop
   });
   mockAcquireXcodebuildSimulatorSetRedirect.mockImplementation(async () => {
     await gate;
-    return { release: mockRedirectRelease };
+    return { release: mockRedirectRelease, releaseBestEffort: mockRedirectRelease };
   });
 
   const starting = ensureRunnerSession(device, { speculative: true });
@@ -215,7 +218,7 @@ test('a release that waits out a demanded start leaves that runner alone', async
   });
   mockAcquireXcodebuildSimulatorSetRedirect.mockImplementation(async () => {
     await gate;
-    return { release: mockRedirectRelease };
+    return { release: mockRedirectRelease, releaseBestEffort: mockRedirectRelease };
   });
 
   const starting = ensureRunnerSession(device, {});

packages/platform-apple/src/runner/__tests__/runner-session-stale-bundles.test.ts

@@ -146,7 +146,10 @@ beforeEach(async () => {
   });
   mockResolveExpectedRunnerCacheMetadata.mockReturnValue({ schemaVersion: 1 });
   mockResolveRunnerDerivedPath.mockReturnValue('/tmp/derived');
-  mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue({ release: mockRedirectRelease });
+  mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue({
+    release: mockRedirectRelease,
+    releaseBestEffort: mockRedirectRelease,
+  });
   mockRunCmdBackground.mockReturnValue(makeBackgroundRunner(4242));
   mockRunAppleToolCommand.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' });
   mockIsProcessAlive.mockReturnValue(true);

packages/platform-apple/src/runner/__tests__/runner-session.test.ts

@@ -192,7 +192,10 @@ beforeEach(async () => {
   });
   mockResolveExpectedRunnerCacheMetadata.mockReturnValue({ schemaVersion: 1 });
   mockResolveRunnerDerivedPath.mockReturnValue('/tmp/derived');
-  mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue({ release: mockRedirectRelease });
+  mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue({
+    release: mockRedirectRelease,
+    releaseBestEffort: mockRedirectRelease,
+  });
   mockRunCmdBackground.mockReturnValue(makeBackgroundRunner(4242));
   mockRunAppleToolCommand.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' });
   mockIsProcessAlive.mockReturnValue(true);
@@ -700,7 +703,10 @@ test('runner session emits XCTest startup progress only after a runner rebuild',
     xctestrunPath: '/tmp/session-runner.xctestrun',
     jsonPath: '/tmp/session-runner.json',
   });
-  mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue({ release: mockRedirectRelease });
+  mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue({
+    release: mockRedirectRelease,
+    releaseBestEffort: mockRedirectRelease,
+  });
   mockRunCmdBackground.mockReturnValue(makeBackgroundRunner(4242));
   mockWaitForRunner.mockResolvedValue(runnerResponse({ uptimeMs: 1 }));
 

packages/platform-apple/src/runner/runner-device-set.ts

@@ -20,7 +20,15 @@ const XCTEST_DEVICE_SET_LOCK_POLL_MS = 100;
 const XCTEST_DEVICE_SET_LOCK_OWNER_GRACE_MS = 5_000;
 
 export type XcodebuildSimulatorSetRedirectHandle = {
+  /** Reconciles the host's device set and gives the lock back, reporting whatever goes wrong. */
   release: () => Promise<void>;
+  /**
+   * Gives the redirect back for a caller whose own outcome is already decided — a launch that
+   * failed, a teardown that ran — and so has nothing left to displace. Only a release that cannot
+   * verify ownership is dropped: that claim is spent, and a later reclaim reads it as dead. A
+   * failure to restore the host's own `XCTestDevices` is not that, and is not swallowed.
+   */
+  releaseBestEffort: () => Promise<void>;
 };
 
 type XcodebuildSimulatorSetRedirectOptions = {
@@ -62,21 +70,6 @@ export async function withXcodebuildSimulatorSetRedirect<Task>(
   return await withProcessLock({ acquire: async () => redirect.release, task });
 }
 
-/**
- * Gives a redirect back from a site that outlives a single task — a launch that already failed, a
- * teardown that already ran — and so has nothing left to displace. A release that cannot verify
- * ownership leaves the lock standing for the stale-clear path, which is the smaller loss.
- */
-export async function releaseXcodebuildSimulatorSetRedirectBestEffort(
-  redirect: XcodebuildSimulatorSetRedirectHandle | null | undefined,
-): Promise<void> {
-  try {
-    await redirect?.release();
-  } catch {
-    // The lock stays where it is; nobody but the stale-clear path may take it from here.
-  }
-}
-
 export async function acquireXcodebuildSimulatorSetRedirect(
   device: DeviceInfo,
   options: XcodebuildSimulatorSetRedirectOptions = {},
@@ -143,24 +136,40 @@ export async function acquireXcodebuildSimulatorSetRedirect(
   }
 
   let released = false;
+  const release = async () => {
+    if (released) {
+      return;
+    }
+    released = true;
+    try {
+      reconcileXcodebuildSimulatorSetRedirect({
+        xctestDeviceSetPath,
+        backupPath,
+      });
+    } finally {
+      await releaseLock();
+    }
+  };
   return {
-    release: async () => {
-      if (released) {
-        return;
-      }
-      released = true;
+    release,
+    releaseBestEffort: async () => {
       try {
-        reconcileXcodebuildSimulatorSetRedirect({
-          xctestDeviceSetPath,
-          backupPath,
-        });
-      } finally {
-        await releaseLock();
+        await release();
+      } catch (error) {
+        // The one failure a caller with nothing left to report may drop. The lock stands under a
+        // claim that has since been spent, which the next reclaim from this process reads as dead,
+        // and `releaseProcessLock` has already recorded it in the request log. Anything else — a
+        // restore of the host's own device set that could not run — is a fact about this machine.
+        if (!isOwnerReleaseUnverified(error)) throw error;
       }
     },
   };
 }
 
+function isOwnerReleaseUnverified(error: unknown): boolean {
+  return error instanceof AppError && error.details?.ownerReleaseUnverified === true;
+}
+
 // fallow-ignore-next-line complexity
 function reconcileXcodebuildSimulatorSetRedirect(paths: {
   xctestDeviceSetPath: string;

packages/platform-apple/src/runner/runner-disposal.ts

@@ -11,7 +11,6 @@ import {
 } from './host.ts';
 import { isMacOs, type DeviceInfo } from '@agent-device/kernel/device';
 import { cleanupTempFile } from './runner-io.ts';
-import { releaseXcodebuildSimulatorSetRedirectBestEffort } from './runner-device-set.ts';
 import { waitForRunner } from './runner-startup-transport.ts';
 import { withRunnerCommandId, type RunnerCommand } from './runner-contract.ts';
 import {
@@ -192,7 +191,7 @@ async function cleanupRunnerSessionResources(
   await settleOwnedRunnerDeviceState(session, options);
   cleanupTempFile(session.xctestrunPath);
   cleanupTempFile(session.jsonPath);
-  await releaseXcodebuildSimulatorSetRedirectBestEffort(session.simulatorSetRedirect);
+  await session.simulatorSetRedirect?.releaseBestEffort();
 }
 
 /**

packages/platform-apple/src/runner/runner-session-types.ts

@@ -39,7 +39,11 @@ export type RunnerSession = {
   startupTimings?: Record<string, number>;
   startupTimingsReported?: boolean;
   logicalLeaseContext?: RunnerLogicalLeaseContext;
-  simulatorSetRedirect?: { release: () => Promise<void> };
+  /** `XcodebuildSimulatorSetRedirectHandle`, seen through the two operations a session performs. */
+  simulatorSetRedirect?: {
+    release: () => Promise<void>;
+    releaseBestEffort: () => Promise<void>;
+  };
   lease?: RunnerLease;
 };
 

packages/platform-apple/src/runner/runner-session.ts

@@ -14,7 +14,6 @@ import { isIosFamily, isApplePlatform, type DeviceInfo } from '@agent-device/ker
 import type { RunnerLogicalLeaseContext } from '@agent-device/contracts/runner-lease-context';
 import type { AppleRunnerLifecycleOptions } from './runner-provider.ts';
 import { getFreePort } from './runner-io.ts';
-import { releaseXcodebuildSimulatorSetRedirectBestEffort } from './runner-device-set.ts';
 import { waitForRunner, RUNNER_STARTUP_TIMEOUT_MS } from './runner-startup-transport.ts';
 import { sendRunnerCommandOnce } from './runner-transport.ts';
 import {
@@ -248,7 +247,7 @@ async function startRunnerSessionWithLease(
       }),
     );
   } catch (error) {
-    await releaseXcodebuildSimulatorSetRedirectBestEffort(simulatorSetRedirect);
+    await simulatorSetRedirect?.releaseBestEffort();
     throw error;
   }
   const sessionId = buildRunnerSessionId(device.id, port);