From c1bf2089a0b1979ab79cb309bb86a0d9491be947 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 7 Aug 2026 13:55:40 +0200 Subject: [PATCH 1/3] fix(ios): heal dangling XCTestDevices symlink during device-set reconcile fs.existsSync follows symlinks, so a redirect symlink whose target set was deleted read as absent: reconcile skipped the unlink and the backup rename failed with ENOTDIR, wedging every XCTest command on the machine until manual cleanup. Detect symlinks with lstat (throwIfNoEntry) in both the reconcile check and unlinkIfSymlink so the stale link is removed and the backup restored. --- .../core/__tests__/runner-xctestrun.test.ts | 29 +++++++++++++++++++ .../apple/core/runner/runner-device-set.ts | 21 +++++++------- 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/src/platforms/apple/core/__tests__/runner-xctestrun.test.ts b/src/platforms/apple/core/__tests__/runner-xctestrun.test.ts index 49202164f2..5b331998a7 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 1f790c434b..306eefee7a 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 { From 126e309fd4a1029da18e3114b651031baba1f502 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 7 Aug 2026 13:55:41 +0200 Subject: [PATCH 2/3] fix: prove owner death before reclaiming locks, and detect zombies A zombified lock owner passes kill(pid, 0) and still reports its original lstart, so a killed-but-unreaped daemon held the XCTest device-set lock forever. Conversely a ps read lost to CPU contention condemned a live owner and let waiters steal a held lock (the flake pinOwnProcessStartTime papers over in tests). Unify both liveness surfaces on classifyOwnerLiveness: condemn zombies via ps state, treat failed ps reads as no-proof rather than death, and reclaim null-start-time owners whose pid provably started after the lock was acquired (ps etime bound). Lock timeout errors now report ownerLiveness. --- .../__tests__/host-process-liveness.test.ts | 58 +++++++++++++ .../__tests__/owner-identity-liveness.test.ts | 86 +++++++++++++++++++ src/utils/__tests__/process-lock.test.ts | 65 +++++++++++++- src/utils/host-process.ts | 27 +++++- src/utils/owner-identity.ts | 42 +++++++-- src/utils/process-lock.ts | 16 ++-- 6 files changed, 273 insertions(+), 21 deletions(-) create mode 100644 src/utils/__tests__/host-process-liveness.test.ts create mode 100644 src/utils/__tests__/owner-identity-liveness.test.ts 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 0000000000..4243ef7d46 --- /dev/null +++ b/src/utils/__tests__/host-process-liveness.test.ts @@ -0,0 +1,58 @@ +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, readProcessStartedAtMs } 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('readProcessStartedAtMs derives the start from mm:ss elapsed time', () => { + psReturns(' 04:05\n'); + assert.equal(readProcessStartedAtMs(4242, 1_000_000), 1_000_000 - (4 * 60 + 5) * 1_000); +}); + +test('readProcessStartedAtMs handles hh:mm:ss and dd-hh:mm:ss elapsed formats', () => { + psReturns('1:02:03\n'); + assert.equal(readProcessStartedAtMs(4242, 10_000_000), 10_000_000 - 3_723_000); + psReturns('18-13:56:27\n'); + assert.equal( + readProcessStartedAtMs(4242, 2_000_000_000), + 2_000_000_000 - (((18 * 24 + 13) * 60 + 56) * 60 + 27) * 1_000, + ); +}); + +test('readProcessStartedAtMs returns null for unparseable or failed ps output', () => { + psReturns('garbage\n'); + assert.equal(readProcessStartedAtMs(4242, 1_000_000), null); + psReturns('', 1); + assert.equal(readProcessStartedAtMs(4242, 1_000_000), null); + assert.equal(readProcessStartedAtMs(-1, 1_000_000), null); +}); 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 0000000000..8912e068a1 --- /dev/null +++ b/src/utils/__tests__/owner-identity-liveness.test.ts @@ -0,0 +1,86 @@ +import assert from 'node:assert/strict'; +import { beforeEach, test, vi } from 'vitest'; + +const { + mockIsProcessAlive, + mockIsProcessZombie, + mockReadProcessStartTime, + mockReadProcessStartedAtMs, +} = vi.hoisted(() => ({ + mockIsProcessAlive: vi.fn(), + mockIsProcessZombie: vi.fn(), + mockReadProcessStartTime: vi.fn(), + mockReadProcessStartedAtMs: vi.fn(), +})); + +vi.mock('../host-process.ts', () => ({ + isProcessAlive: mockIsProcessAlive, + isProcessZombie: mockIsProcessZombie, + readProcessStartTime: mockReadProcessStartTime, + readProcessStartedAtMs: mockReadProcessStartedAtMs, +})); + +import { classifyOwnerLiveness } from '../owner-identity.ts'; + +const OWNER_PID = 4242; + +beforeEach(() => { + mockIsProcessAlive.mockReset().mockReturnValue(true); + mockIsProcessZombie.mockReset().mockReturnValue(false); + mockReadProcessStartTime.mockReset().mockReturnValue('start-a'); + mockReadProcessStartedAtMs.mockReset().mockReturnValue(null); +}); + +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('null recorded start time stays live without an acquisition bound', () => { + mockReadProcessStartedAtMs.mockReturnValue(Date.now()); + assert.equal(classifyOwnerLiveness({ owner: { pid: OWNER_PID, startTime: null } }), 'live'); +}); + +test('null recorded start time dies when the pid started after acquisition', () => { + const acquiredAtMs = 1_000_000; + mockReadProcessStartedAtMs.mockReturnValue(acquiredAtMs + 120_000); + assert.equal( + classifyOwnerLiveness({ owner: { pid: OWNER_PID, startTime: null }, acquiredAtMs }), + 'owner-process-dead', + ); +}); + +test('null recorded start time stays live when the pid started before acquisition or within slack', () => { + const acquiredAtMs = 1_000_000; + mockReadProcessStartedAtMs.mockReturnValue(acquiredAtMs - 5_000); + assert.equal( + classifyOwnerLiveness({ owner: { pid: OWNER_PID, startTime: null }, acquiredAtMs }), + 'live', + ); + mockReadProcessStartedAtMs.mockReturnValue(acquiredAtMs + 5_000); + assert.equal( + classifyOwnerLiveness({ owner: { pid: OWNER_PID, startTime: null }, acquiredAtMs }), + 'live', + ); + mockReadProcessStartedAtMs.mockReturnValue(null); + assert.equal( + classifyOwnerLiveness({ owner: { pid: OWNER_PID, startTime: null }, acquiredAtMs }), + 'live', + ); +}); diff --git a/src/utils/__tests__/process-lock.test.ts b/src/utils/__tests__/process-lock.test.ts index 7b02b94804..7a5a143b73 100644 --- a/src/utils/__tests__/process-lock.test.ts +++ b/src/utils/__tests__/process-lock.test.ts @@ -1,10 +1,18 @@ 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 { readProcessStartedAtMs, readProcessStartTime } from '../host-process.ts'; import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts'; let tmpDir: string; @@ -54,6 +62,58 @@ 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 reclaims a null-start-time lock whose pid started after acquisition', async () => { + const ownStartedAtMs = readProcessStartedAtMs(process.pid); + if (ownStartedAtMs === null) { + // ps lost to CPU contention; the classification itself is covered by + // owner-identity-liveness.test.ts. + return; + } + const lockDirPath = path.join(tmpDir, 'recycled.lock'); + fs.mkdirSync(lockDirPath); + // Recorded before this process started, with no start-time identity: the + // pid provably belongs to a later process, so the lock is reclaimable. + fs.writeFileSync( + path.join(lockDirPath, 'owner.json'), + JSON.stringify({ + pid: process.pid, + startTime: null, + acquiredAtMs: ownStartedAtMs - 10 * 60_000, + }), + ); + + const release = await acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + timeoutMs: 3_000, + pollMs: 1, + }); + await release(); + assert.equal(fs.existsSync(lockDirPath), false); +}); + test('acquireProcessLock reports live lock owner details on timeout', async () => { const lockDirPath = path.join(tmpDir, 'busy.lock'); fs.mkdirSync(lockDirPath); @@ -75,6 +135,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 97806bb051..fe920e3093 100644 --- a/src/utils/host-process.ts +++ b/src/utils/host-process.ts @@ -55,7 +55,32 @@ 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; +} + +export function readProcessStartedAtMs(pid: number, nowMs: number = Date.now()): number | null { + const elapsedMs = parseEtimeMs(readProcessField(pid, 'etime=')); + return elapsedMs === null ? null : nowMs - elapsedMs; +} + +function parseEtimeMs(value: string | null): number | null { + if (!value) return null; + const match = /^(?:(\d+)-)?(?:(\d+):)?(\d{1,2}):(\d{2})$/.exec(value); + if (!match) return null; + const [, days = '0', hours = '0', minutes, seconds] = match; + return ( + (Number(days) * 86_400 + Number(hours) * 3_600 + Number(minutes) * 60 + Number(seconds)) * 1_000 + ); +} + +function readProcessField( + pid: number, + field: 'lstart=' | 'command=' | 'state=' | 'etime=', +): 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 2d3e7ad113..c1ce3daa14 100644 --- a/src/utils/owner-identity.ts +++ b/src/utils/owner-identity.ts @@ -1,5 +1,15 @@ import fs from 'node:fs'; -import { isProcessAlive, readProcessStartTime } from './host-process.ts'; +import { + isProcessAlive, + isProcessZombie, + readProcessStartedAtMs, + readProcessStartTime, +} from './host-process.ts'; + +// ps reports process start times at second granularity and the host clock can +// step; a current start time must clear acquiredAtMs by this margin before it +// proves the pid was recycled. +const OWNER_START_TIME_SLACK_MS = 30_000; export type OwnerIdentity = { pid: number; @@ -20,18 +30,36 @@ 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), its start time differs from the recorded one, + * or it started after `acquiredAtMs` — a process born after the resource was + * acquired cannot be the acquirer, which catches recycled pids even when the + * owner's start time was never recorded. */ export function classifyOwnerLiveness(params: { owner: Pick; stateDir?: string; + acquiredAtMs?: number; }): OwnerLiveness { - const { owner, stateDir } = params; + const { owner, stateDir, acquiredAtMs } = 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'; + } + } else if (acquiredAtMs !== undefined) { + const startedAtMs = readProcessStartedAtMs(owner.pid); + if (startedAtMs !== null && startedAtMs > acquiredAtMs + OWNER_START_TIME_SLACK_MS) { + return 'owner-process-dead'; + } } if (!stateDir) return 'live'; try { diff --git a/src/utils/process-lock.ts b/src/utils/process-lock.ts index a5c651d6ad..3eac0c337f 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,14 @@ function readProcessLockDiagnostics( ownerPid: owner.pid, ownerStartTime: owner.startTime, ownerAgeMs: Math.max(0, Math.round(nowMs - owner.acquiredAtMs)), + ownerLiveness: classifyOwnerLiveness({ owner, acquiredAtMs: owner.acquiredAtMs }), } : {}), }; } 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, acquiredAtMs: owner.acquiredAtMs }) !== 'owner-process-dead' + ); } From 9af92d58fe1d6756096fe72c5c03814658df8c2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 7 Aug 2026 14:17:56 +0200 Subject: [PATCH 3/3] fix: keep null-start-time lock owners fail-closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review P1 on #1672: the started-after-acquisition bound compared a wall-clock birth estimate (Date.now() - ps etime) against the persisted wall-clock acquiredAtMs, so a clock step larger than the fixed 30s slack could condemn a live null-start owner and let a waiter steal the held lock — the failure class this PR eliminates. There is no same-clock-domain proof of birth order to be had here (macOS ps etime is itself wall-derived), so drop the bound: an alive pid with no recorded start time is never reclaimed. Regression covers the steal at both the classifier and the lock level. --- .../__tests__/host-process-liveness.test.ts | 26 ++--------- .../__tests__/owner-identity-liveness.test.ts | 44 +++---------------- src/utils/__tests__/process-lock.test.ts | 41 ++++++++--------- src/utils/host-process.ts | 20 +-------- src/utils/owner-identity.ts | 29 +++--------- src/utils/process-lock.ts | 6 +-- 6 files changed, 40 insertions(+), 126 deletions(-) diff --git a/src/utils/__tests__/host-process-liveness.test.ts b/src/utils/__tests__/host-process-liveness.test.ts index 4243ef7d46..74b29ab8c2 100644 --- a/src/utils/__tests__/host-process-liveness.test.ts +++ b/src/utils/__tests__/host-process-liveness.test.ts @@ -8,7 +8,7 @@ vi.mock('../exec.ts', async () => { return { ...actual, runCmdSync: mockRunCmdSync }; }); -import { isProcessZombie, readProcessStartedAtMs } from '../host-process.ts'; +import { isProcessZombie } from '../host-process.ts'; function psReturns(stdout: string, exitCode = 0): void { mockRunCmdSync.mockReturnValue({ stdout, stderr: '', exitCode }); @@ -34,25 +34,7 @@ test('isProcessZombie treats running states and ps failures as not zombie', () = assert.equal(isProcessZombie(4242), false); }); -test('readProcessStartedAtMs derives the start from mm:ss elapsed time', () => { - psReturns(' 04:05\n'); - assert.equal(readProcessStartedAtMs(4242, 1_000_000), 1_000_000 - (4 * 60 + 5) * 1_000); -}); - -test('readProcessStartedAtMs handles hh:mm:ss and dd-hh:mm:ss elapsed formats', () => { - psReturns('1:02:03\n'); - assert.equal(readProcessStartedAtMs(4242, 10_000_000), 10_000_000 - 3_723_000); - psReturns('18-13:56:27\n'); - assert.equal( - readProcessStartedAtMs(4242, 2_000_000_000), - 2_000_000_000 - (((18 * 24 + 13) * 60 + 56) * 60 + 27) * 1_000, - ); -}); - -test('readProcessStartedAtMs returns null for unparseable or failed ps output', () => { - psReturns('garbage\n'); - assert.equal(readProcessStartedAtMs(4242, 1_000_000), null); - psReturns('', 1); - assert.equal(readProcessStartedAtMs(4242, 1_000_000), null); - assert.equal(readProcessStartedAtMs(-1, 1_000_000), null); +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 index 8912e068a1..521861ad96 100644 --- a/src/utils/__tests__/owner-identity-liveness.test.ts +++ b/src/utils/__tests__/owner-identity-liveness.test.ts @@ -1,23 +1,16 @@ import assert from 'node:assert/strict'; import { beforeEach, test, vi } from 'vitest'; -const { - mockIsProcessAlive, - mockIsProcessZombie, - mockReadProcessStartTime, - mockReadProcessStartedAtMs, -} = vi.hoisted(() => ({ +const { mockIsProcessAlive, mockIsProcessZombie, mockReadProcessStartTime } = vi.hoisted(() => ({ mockIsProcessAlive: vi.fn(), mockIsProcessZombie: vi.fn(), mockReadProcessStartTime: vi.fn(), - mockReadProcessStartedAtMs: vi.fn(), })); vi.mock('../host-process.ts', () => ({ isProcessAlive: mockIsProcessAlive, isProcessZombie: mockIsProcessZombie, readProcessStartTime: mockReadProcessStartTime, - readProcessStartedAtMs: mockReadProcessStartedAtMs, })); import { classifyOwnerLiveness } from '../owner-identity.ts'; @@ -28,7 +21,6 @@ beforeEach(() => { mockIsProcessAlive.mockReset().mockReturnValue(true); mockIsProcessZombie.mockReset().mockReturnValue(false); mockReadProcessStartTime.mockReset().mockReturnValue('start-a'); - mockReadProcessStartedAtMs.mockReset().mockReturnValue(null); }); test('classifies a zombie owner as owner-process-dead despite a matching start time', () => { @@ -52,35 +44,9 @@ test('a definite start-time mismatch classifies as owner-process-dead', () => { ); }); -test('null recorded start time stays live without an acquisition bound', () => { - mockReadProcessStartedAtMs.mockReturnValue(Date.now()); +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'); }); - -test('null recorded start time dies when the pid started after acquisition', () => { - const acquiredAtMs = 1_000_000; - mockReadProcessStartedAtMs.mockReturnValue(acquiredAtMs + 120_000); - assert.equal( - classifyOwnerLiveness({ owner: { pid: OWNER_PID, startTime: null }, acquiredAtMs }), - 'owner-process-dead', - ); -}); - -test('null recorded start time stays live when the pid started before acquisition or within slack', () => { - const acquiredAtMs = 1_000_000; - mockReadProcessStartedAtMs.mockReturnValue(acquiredAtMs - 5_000); - assert.equal( - classifyOwnerLiveness({ owner: { pid: OWNER_PID, startTime: null }, acquiredAtMs }), - 'live', - ); - mockReadProcessStartedAtMs.mockReturnValue(acquiredAtMs + 5_000); - assert.equal( - classifyOwnerLiveness({ owner: { pid: OWNER_PID, startTime: null }, acquiredAtMs }), - 'live', - ); - mockReadProcessStartedAtMs.mockReturnValue(null); - assert.equal( - classifyOwnerLiveness({ owner: { pid: OWNER_PID, startTime: null }, acquiredAtMs }), - 'live', - ); -}); diff --git a/src/utils/__tests__/process-lock.test.ts b/src/utils/__tests__/process-lock.test.ts index 7a5a143b73..af3399fad7 100644 --- a/src/utils/__tests__/process-lock.test.ts +++ b/src/utils/__tests__/process-lock.test.ts @@ -12,7 +12,7 @@ vi.mock('../host-process.ts', async (importOriginal) => { }); import { acquireProcessLock, type ProcessLockOwner } from '../process-lock.ts'; -import { readProcessStartedAtMs, readProcessStartTime } from '../host-process.ts'; +import { readProcessStartTime } from '../host-process.ts'; import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts'; let tmpDir: string; @@ -84,34 +84,35 @@ test('acquireProcessLock reclaims locks owned by zombie processes', async () => } }); -test('acquireProcessLock reclaims a null-start-time lock whose pid started after acquisition', async () => { - const ownStartedAtMs = readProcessStartedAtMs(process.pid); - if (ownStartedAtMs === null) { - // ps lost to CPU contention; the classification itself is covered by - // owner-identity-liveness.test.ts. - return; - } - const lockDirPath = path.join(tmpDir, 'recycled.lock'); +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); - // Recorded before this process started, with no start-time identity: the - // pid provably belongs to a later process, so the lock is reclaimable. + // 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: ownStartedAtMs - 10 * 60_000, + acquiredAtMs: Date.now() - 365 * 24 * 60 * 60_000, }), ); - const release = await acquireProcessLock({ - lockDirPath, - owner: currentProcessOwner(), - timeoutMs: 3_000, - pollMs: 1, - }); - await release(); - assert.equal(fs.existsSync(lockDirPath), false); + 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 () => { diff --git a/src/utils/host-process.ts b/src/utils/host-process.ts index fe920e3093..1ad49a1879 100644 --- a/src/utils/host-process.ts +++ b/src/utils/host-process.ts @@ -62,25 +62,7 @@ export function isProcessZombie(pid: number): boolean { return readProcessField(pid, 'state=')?.startsWith('Z') ?? false; } -export function readProcessStartedAtMs(pid: number, nowMs: number = Date.now()): number | null { - const elapsedMs = parseEtimeMs(readProcessField(pid, 'etime=')); - return elapsedMs === null ? null : nowMs - elapsedMs; -} - -function parseEtimeMs(value: string | null): number | null { - if (!value) return null; - const match = /^(?:(\d+)-)?(?:(\d+):)?(\d{1,2}):(\d{2})$/.exec(value); - if (!match) return null; - const [, days = '0', hours = '0', minutes, seconds] = match; - return ( - (Number(days) * 86_400 + Number(hours) * 3_600 + Number(minutes) * 60 + Number(seconds)) * 1_000 - ); -} - -function readProcessField( - pid: number, - field: 'lstart=' | 'command=' | 'state=' | 'etime=', -): string | null { +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 c1ce3daa14..2562bd50c3 100644 --- a/src/utils/owner-identity.ts +++ b/src/utils/owner-identity.ts @@ -1,15 +1,5 @@ import fs from 'node:fs'; -import { - isProcessAlive, - isProcessZombie, - readProcessStartedAtMs, - readProcessStartTime, -} from './host-process.ts'; - -// ps reports process start times at second granularity and the host clock can -// step; a current start time must clear acquiredAtMs by this margin before it -// proves the pid was recycled. -const OWNER_START_TIME_SLACK_MS = 30_000; +import { isProcessAlive, isProcessZombie, readProcessStartTime } from './host-process.ts'; export type OwnerIdentity = { pid: number; @@ -37,17 +27,17 @@ export function ownerIdentityMatches( * 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), its start time differs from the recorded one, - * or it started after `acquiredAtMs` — a process born after the resource was - * acquired cannot be the acquirer, which catches recycled pids even when the - * owner's start time was never recorded. + * 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; stateDir?: string; - acquiredAtMs?: number; }): OwnerLiveness { - const { owner, stateDir, acquiredAtMs } = params; + const { owner, stateDir } = params; if (!isProcessAlive(owner.pid)) return 'owner-process-dead'; if (isProcessZombie(owner.pid)) return 'owner-process-dead'; if (owner.startTime) { @@ -55,11 +45,6 @@ export function classifyOwnerLiveness(params: { if (currentStartTime !== null && currentStartTime !== owner.startTime) { return 'owner-process-dead'; } - } else if (acquiredAtMs !== undefined) { - const startedAtMs = readProcessStartedAtMs(owner.pid); - if (startedAtMs !== null && startedAtMs > acquiredAtMs + OWNER_START_TIME_SLACK_MS) { - return 'owner-process-dead'; - } } if (!stateDir) return 'live'; try { diff --git a/src/utils/process-lock.ts b/src/utils/process-lock.ts index 3eac0c337f..07e97aec6a 100644 --- a/src/utils/process-lock.ts +++ b/src/utils/process-lock.ts @@ -117,14 +117,12 @@ function readProcessLockDiagnostics( ownerPid: owner.pid, ownerStartTime: owner.startTime, ownerAgeMs: Math.max(0, Math.round(nowMs - owner.acquiredAtMs)), - ownerLiveness: classifyOwnerLiveness({ owner, acquiredAtMs: owner.acquiredAtMs }), + ownerLiveness: classifyOwnerLiveness({ owner }), } : {}), }; } function isLiveProcessLockOwner(owner: ProcessLockOwner): boolean { - return ( - classifyOwnerLiveness({ owner, acquiredAtMs: owner.acquiredAtMs }) !== 'owner-process-dead' - ); + return classifyOwnerLiveness({ owner }) !== 'owner-process-dead'; }