diff --git a/src/platforms/apple/core/__tests__/runner-xctestrun.test.ts b/src/platforms/apple/core/__tests__/runner-xctestrun.test.ts index 49202164f..5b331998a 100644 --- a/src/platforms/apple/core/__tests__/runner-xctestrun.test.ts +++ b/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> | 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> | null = null; await withTempDir('runner-xctestrun-redirect-', async (root) => { diff --git a/src/platforms/apple/core/runner/runner-device-set.ts b/src/platforms/apple/core/runner/runner-device-set.ts index 1f790c434..306eefee7 100644 --- a/src/platforms/apple/core/runner/runner-device-set.ts +++ b/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 { diff --git a/src/utils/__tests__/host-process-liveness.test.ts b/src/utils/__tests__/host-process-liveness.test.ts new file mode 100644 index 000000000..74b29ab8c --- /dev/null +++ b/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('../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); +}); diff --git a/src/utils/__tests__/owner-identity-liveness.test.ts b/src/utils/__tests__/owner-identity-liveness.test.ts new file mode 100644 index 000000000..521861ad9 --- /dev/null +++ b/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'); +}); diff --git a/src/utils/__tests__/process-lock.test.ts b/src/utils/__tests__/process-lock.test.ts index 7b02b9480..af3399fad 100644 --- a/src/utils/__tests__/process-lock.test.ts +++ b/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() })); + +vi.mock('../host-process.ts', async (importOriginal) => { + const actual = await importOriginal(); + 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; }, ); diff --git a/src/utils/host-process.ts b/src/utils/host-process.ts index 97806bb05..1ad49a187 100644 --- a/src/utils/host-process.ts +++ b/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], { diff --git a/src/utils/owner-identity.ts b/src/utils/owner-identity.ts index 2d3e7ad11..2562bd50c 100644 --- a/src/utils/owner-identity.ts +++ b/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,9 +20,18 @@ 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; @@ -30,8 +39,12 @@ export function classifyOwnerLiveness(params: { }): 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 { diff --git a/src/utils/process-lock.ts b/src/utils/process-lock.ts index a5c651d6a..07e97aec6 100644 --- a/src/utils/process-lock.ts +++ b/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'; }