fix: heal dangling XCTestDevices redirect and prove lock-owner death before reclaim

src/platforms/apple/core/__tests__/runner-xctestrun.test.ts

@@ -528,6 +528,35 @@ test('acquireXcodebuildSimulatorSetRedirect restores stale redirected XCTestDevi
   });
 });
 
+test('acquireXcodebuildSimulatorSetRedirect restores the backup when XCTestDevices is a dangling symlink', async () => {
+  let handle: Awaited<ReturnType<typeof acquireXcodebuildSimulatorSetRedirect>> | null = null;
+  await withTempDir('runner-xctestrun-redirect-', async (root) => {
+    const paths = makeRedirectPaths(root);
+    fs.mkdirSync(paths.requestedSetPath, { recursive: true });
+    fs.mkdirSync(path.dirname(paths.xctestDeviceSetPath), { recursive: true });
+    fs.mkdirSync(paths.backupPath, { recursive: true });
+    fs.writeFileSync(path.join(paths.backupPath, 'original.txt'), 'restored', 'utf8');
+    // Stale redirect whose target set was deleted by its caller.
+    fs.symlinkSync(path.join(root, 'deleted-requested'), paths.xctestDeviceSetPath, 'dir');
+
+    handle = await acquireRedirect(paths, { backupPath: paths.backupPath });
+
+    assert.notEqual(handle, null);
+    assertRedirectTargetsRequestedSet(paths);
+
+    await handle?.release();
+    handle = null;
+
+    assert.equal(fs.existsSync(paths.backupPath), false);
+    assert.equal(
+      fs.readFileSync(path.join(paths.xctestDeviceSetPath, 'original.txt'), 'utf8'),
+      'restored',
+    );
+  }).finally(async () => {
+    await handle?.release();
+  });
+});
+
 test('acquireXcodebuildSimulatorSetRedirect clears stale lock directories from dead owners', async () => {
   let handle: Awaited<ReturnType<typeof acquireXcodebuildSimulatorSetRedirect>> | null = null;
   await withTempDir('runner-xctestrun-redirect-', async (root) => {

src/platforms/apple/core/runner/runner-device-set.ts

@@ -132,8 +132,7 @@ function reconcileXcodebuildSimulatorSetRedirect(paths: {
   const { xctestDeviceSetPath, backupPath } = paths;
   const existingBackups = [backupPath, ...findLegacyXcodebuildSimulatorSetBackups(backupPath)];
   const activeBackupPath = existingBackups.find((candidate) => fs.existsSync(candidate));
-  const xctestExists = fs.existsSync(xctestDeviceSetPath);
-  const xctestIsSymlink = xctestExists && fs.lstatSync(xctestDeviceSetPath).isSymbolicLink();
+  const xctestIsSymlink = isSymlink(xctestDeviceSetPath);
 
   if (activeBackupPath) {
     if (xctestIsSymlink) {
@@ -210,21 +209,21 @@ function installXcodebuildSimulatorSetSymlink(paths: {
     fs.symlinkSync(requestedSetPath, tmpSymlinkPath, 'dir');
     fs.renameSync(tmpSymlinkPath, xctestDeviceSetPath);
   } catch (error) {
-    if (fs.existsSync(tmpSymlinkPath)) {
-      unlinkIfSymlink(tmpSymlinkPath);
-    }
+    unlinkIfSymlink(tmpSymlinkPath);
     throw error;
   }
 }
 
+// lstat instead of existsSync: existsSync follows symlinks, so a dangling
+// symlink (target deleted) would read as absent and never get cleaned up.
+function isSymlink(targetPath: string): boolean {
+  return fs.lstatSync(targetPath, { throwIfNoEntry: false })?.isSymbolicLink() ?? false;
+}
+
 function unlinkIfSymlink(targetPath: string): void {
-  if (!fs.existsSync(targetPath)) {
-    return;
-  }
-  if (!fs.lstatSync(targetPath).isSymbolicLink()) {
-    return;
+  if (isSymlink(targetPath)) {
+    fs.unlinkSync(targetPath);
   }
-  fs.unlinkSync(targetPath);
 }
 
 function sameResolvedPath(left: string, right: string): boolean {

src/utils/__tests__/host-process-liveness.test.ts

@@ -0,0 +1,40 @@
+import assert from 'node:assert/strict';
+import { beforeEach, test, vi } from 'vitest';
+
+const { mockRunCmdSync } = vi.hoisted(() => ({ mockRunCmdSync: vi.fn() }));
+
+vi.mock('../exec.ts', async () => {
+  const actual = await vi.importActual<typeof import('../exec.ts')>('../exec.ts');
+  return { ...actual, runCmdSync: mockRunCmdSync };
+});
+
+import { isProcessZombie } from '../host-process.ts';
+
+function psReturns(stdout: string, exitCode = 0): void {
+  mockRunCmdSync.mockReturnValue({ stdout, stderr: '', exitCode });
+}
+
+beforeEach(() => {
+  mockRunCmdSync.mockReset();
+});
+
+test('isProcessZombie detects the Z state code with trailing flags', () => {
+  psReturns('ZN  \n');
+  assert.equal(isProcessZombie(4242), true);
+});
+
+test('isProcessZombie treats running states and ps failures as not zombie', () => {
+  psReturns('Ss  \n');
+  assert.equal(isProcessZombie(4242), false);
+  psReturns('', 1);
+  assert.equal(isProcessZombie(4242), false);
+  mockRunCmdSync.mockImplementation(() => {
+    throw new Error('ps timed out');
+  });
+  assert.equal(isProcessZombie(4242), false);
+});
+
+test('isProcessZombie returns false for an invalid pid without invoking ps', () => {
+  assert.equal(isProcessZombie(-1), false);
+  assert.equal(mockRunCmdSync.mock.calls.length, 0);
+});

src/utils/__tests__/owner-identity-liveness.test.ts

@@ -0,0 +1,52 @@
+import assert from 'node:assert/strict';
+import { beforeEach, test, vi } from 'vitest';
+
+const { mockIsProcessAlive, mockIsProcessZombie, mockReadProcessStartTime } = vi.hoisted(() => ({
+  mockIsProcessAlive: vi.fn(),
+  mockIsProcessZombie: vi.fn(),
+  mockReadProcessStartTime: vi.fn(),
+}));
+
+vi.mock('../host-process.ts', () => ({
+  isProcessAlive: mockIsProcessAlive,
+  isProcessZombie: mockIsProcessZombie,
+  readProcessStartTime: mockReadProcessStartTime,
+}));
+
+import { classifyOwnerLiveness } from '../owner-identity.ts';
+
+const OWNER_PID = 4242;
+
+beforeEach(() => {
+  mockIsProcessAlive.mockReset().mockReturnValue(true);
+  mockIsProcessZombie.mockReset().mockReturnValue(false);
+  mockReadProcessStartTime.mockReset().mockReturnValue('start-a');
+});
+
+test('classifies a zombie owner as owner-process-dead despite a matching start time', () => {
+  mockIsProcessZombie.mockReturnValue(true);
+  assert.equal(
+    classifyOwnerLiveness({ owner: { pid: OWNER_PID, startTime: 'start-a' } }),
+    'owner-process-dead',
+  );
+});
+
+test('a failed start-time read is not proof of death for an alive pid', () => {
+  mockReadProcessStartTime.mockReturnValue(null);
+  assert.equal(classifyOwnerLiveness({ owner: { pid: OWNER_PID, startTime: 'start-a' } }), 'live');
+});
+
+test('a definite start-time mismatch classifies as owner-process-dead', () => {
+  mockReadProcessStartTime.mockReturnValue('start-b');
+  assert.equal(
+    classifyOwnerLiveness({ owner: { pid: OWNER_PID, startTime: 'start-a' } }),
+    'owner-process-dead',
+  );
+});
+
+test('a null-start-time owner stays fail-closed while its pid is alive', () => {
+  // No same-clock-domain proof of birth order exists for a null-start owner:
+  // a clock step could make a live owner look like it started after the
+  // resource was acquired, so an alive pid must never be condemned on age.
+  assert.equal(classifyOwnerLiveness({ owner: { pid: OWNER_PID, startTime: null } }), 'live');
+});

src/utils/__tests__/process-lock.test.ts

@@ -1,8 +1,16 @@
 import assert from 'node:assert/strict';
 import fs from 'node:fs';
 import path from 'node:path';
-import { afterEach, beforeEach, test } from 'vitest';
+import { afterEach, beforeEach, test, vi } from 'vitest';
 import { AppError } from '@agent-device/kernel/errors';
+
+const { zombiePids } = vi.hoisted(() => ({ zombiePids: new Set<number>() }));
+
+vi.mock('../host-process.ts', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('../host-process.ts')>();
+  return { ...actual, isProcessZombie: (pid: number) => zombiePids.has(pid) };
+});
+
 import { acquireProcessLock, type ProcessLockOwner } from '../process-lock.ts';
 import { readProcessStartTime } from '../host-process.ts';
 import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts';
@@ -54,6 +62,59 @@ test('acquireProcessLock reclaims locks owned by dead processes', async () => {
   assert.equal(fs.existsSync(lockDirPath), false);
 });
 
+test('acquireProcessLock reclaims locks owned by zombie processes', async () => {
+  const lockDirPath = path.join(tmpDir, 'zombie.lock');
+  fs.mkdirSync(lockDirPath);
+  // The owner passes kill(pid, 0) and matches its recorded start time; only
+  // the zombie state reveals it already terminated.
+  fs.writeFileSync(path.join(lockDirPath, 'owner.json'), JSON.stringify(currentProcessOwner()));
+  zombiePids.add(process.pid);
+
+  try {
+    const release = await acquireProcessLock({
+      lockDirPath,
+      owner: currentProcessOwner(),
+      timeoutMs: 3_000,
+      pollMs: 1,
+    });
+    await release();
+    assert.equal(fs.existsSync(lockDirPath), false);
+  } finally {
+    zombiePids.delete(process.pid);
+  }
+});
+
+test('acquireProcessLock never steals a null-start-time lock from an alive pid', async () => {
+  const lockDirPath = path.join(tmpDir, 'null-start.lock');
+  fs.mkdirSync(lockDirPath);
+  // An acquiredAtMs far older than this process simulates what a wall-clock
+  // step makes a live null-start owner look like; age is not proof of death,
+  // so the waiter must time out instead of reclaiming the held lock.
+  fs.writeFileSync(
+    path.join(lockDirPath, 'owner.json'),
+    JSON.stringify({
+      pid: process.pid,
+      startTime: null,
+      acquiredAtMs: Date.now() - 365 * 24 * 60 * 60_000,
+    }),
+  );
+
+  await assert.rejects(
+    () =>
+      acquireProcessLock({
+        lockDirPath,
+        owner: currentProcessOwner(),
+        timeoutMs: 50,
+        pollMs: 1,
+      }),
+    (error: unknown) => {
+      assert.ok(error instanceof AppError);
+      assert.equal(error.details?.ownerLiveness, 'live');
+      return true;
+    },
+  );
+});
+
 test('acquireProcessLock reports live lock owner details on timeout', async () => {
   const lockDirPath = path.join(tmpDir, 'busy.lock');
   fs.mkdirSync(lockDirPath);
@@ -75,6 +136,7 @@ test('acquireProcessLock reports live lock owner details on timeout', async () =
       assert.equal(error.message, 'Timed out waiting for busy test lock');
       assert.equal(error.details?.lockDirPath, lockDirPath);
       assert.equal(error.details?.ownerPid, process.pid);
+      assert.equal(error.details?.ownerLiveness, 'live');
       return true;
     },
   );

src/utils/host-process.ts

@@ -55,7 +55,14 @@ export function readProcessCommand(pid: number): string | null {
   return readProcessField(pid, 'command=');
 }
 
-function readProcessField(pid: number, field: 'lstart=' | 'command='): string | null {
+// A zombie passes kill(pid, 0) and still reports its original lstart, so both
+// isProcessAlive and the start-time identity check read it as live; only the
+// process state exposes that it already terminated.
+export function isProcessZombie(pid: number): boolean {
+  return readProcessField(pid, 'state=')?.startsWith('Z') ?? false;
+}
+
+function readProcessField(pid: number, field: 'lstart=' | 'command=' | 'state='): string | null {
   if (!Number.isInteger(pid) || pid <= 0) return null;
   try {
     const result = runCmdSync('ps', ['-p', String(pid), '-o', field], {

src/utils/owner-identity.ts

@@ -1,5 +1,5 @@
 import fs from 'node:fs';
-import { isProcessAlive, readProcessStartTime } from './host-process.ts';
+import { isProcessAlive, isProcessZombie, readProcessStartTime } from './host-process.ts';
 
 export type OwnerIdentity = {
   pid: number;
@@ -20,18 +20,31 @@ export function ownerIdentityMatches(
 }
 
 /**
- * This is deliberately proof-oriented. A filesystem read error is not proof
- * that an owner state directory disappeared, so callers must surface it as an
- * unknown owner rather than treating the resource as free.
+ * This is deliberately proof-oriented, in both directions. A filesystem read
+ * error is not proof that an owner state directory disappeared, so callers
+ * must surface it as an unknown owner rather than treating the resource as
+ * free. Likewise a failed `ps` read (it shells out with a short timeout and
+ * loses under CPU contention) is not proof the owner died, so it never
+ * condemns a pid that kill(pid, 0) says is alive. Death is only concluded
+ * from positive evidence: the pid is gone, the process is a zombie (already
+ * terminated, merely unreaped), or its start time differs from the recorded
+ * one. An owner recorded without a start time stays fail-closed while its pid
+ * is alive: there is no same-clock-domain proof of birth order (wall-clock
+ * arithmetic over `ps etime` shifts under clock steps), and misreading a live
+ * owner as recycled would let a waiter steal a held resource.
  */
 export function classifyOwnerLiveness(params: {
   owner: Pick<OwnerIdentity, 'pid' | 'startTime'>;
   stateDir?: string;
 }): OwnerLiveness {
   const { owner, stateDir } = params;
   if (!isProcessAlive(owner.pid)) return 'owner-process-dead';
-  if (owner.startTime && readProcessStartTime(owner.pid) !== owner.startTime) {
-    return 'owner-process-dead';
+  if (isProcessZombie(owner.pid)) return 'owner-process-dead';
+  if (owner.startTime) {
+    const currentStartTime = readProcessStartTime(owner.pid);
+    if (currentStartTime !== null && currentStartTime !== owner.startTime) {
+      return 'owner-process-dead';
+    }
   }
   if (!stateDir) return 'live';
   try {

src/utils/process-lock.ts

@@ -1,7 +1,7 @@
 import fs from 'node:fs';
 import path from 'node:path';
 import { AppError } from '@agent-device/kernel/errors';
-import { isProcessAlive, readProcessStartTime } from './host-process.ts';
+import { classifyOwnerLiveness } from './owner-identity.ts';
 import { sleep } from './timeouts.ts';
 
 const DEFAULT_LOCK_TIMEOUT_MS = 30_000;
@@ -117,20 +117,12 @@ function readProcessLockDiagnostics(
           ownerPid: owner.pid,
           ownerStartTime: owner.startTime,
           ownerAgeMs: Math.max(0, Math.round(nowMs - owner.acquiredAtMs)),
+          ownerLiveness: classifyOwnerLiveness({ owner }),
         }
       : {}),
   };
 }
 
 function isLiveProcessLockOwner(owner: ProcessLockOwner): boolean {
-  if (!Number.isInteger(owner.pid) || owner.pid <= 0) {
-    return false;
-  }
-  if (!isProcessAlive(owner.pid)) {
-    return false;
-  }
-  if (owner.startTime) {
-    return readProcessStartTime(owner.pid) === owner.startTime;
-  }
-  return true;
+  return classifyOwnerLiveness({ owner }) !== 'owner-process-dead';
 }