From 1a131ca4f5df19fbef0f1f58a391f7f2fd1172d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 14 Sep 2026 16:39:32 +0200 Subject: [PATCH 01/18] fix(host-kit): process lock proves ownership before release and never evicts a live owner Release removed the lock directory unconditionally, so a contender that reclaimed the lock while its previous holder was away had that holder delete the exclusion belonging to whoever holds it now. Release removes only while the record inside still names the acquirer, and says so with a typed reason when it cannot read far enough to tell, rather than clearing a lock it might not own. A record that could not be read looked exactly like a record that was never written, so a live owner whose owner.json is unreadable or malformed aged out at the grace window and lost a lock it was holding. Only ENOENT now speaks for an unwritten record; any other read failure, and any record that does not name a live-shaped process, is an owner of unknown identity and the lock stands. Both wait paths carry a hint naming the directory, because nothing else in the repository removes it. A stale reclaim moves the abandoned directory aside under a unique `.lock` name before removing it. Two plain removals both report success, because the second is a silent no-op on a path the first already cleared, and both contenders then continue as though they had freed the lock. mkdir's EEXIST, unchanged, still arbitrates the lock itself. A reclaim that fails for a reason the fallback cannot settle waits and retries instead of surfacing an errno, and where the win32 fallback does force a removal it re-reads the record first, because a refused rename means a live contender may have claimed the path since. The allocation store keeps lane locks beside the lanes they guard, so it now recognises any `.lock` directory as a lock instead of only the `.lane.lock` suffix. --- .../src/internal/process-lock.test.ts | 312 ++++++++++++++++++ .../host-kit/src/internal/process-lock.ts | 184 +++++++++-- .../src/__tests__/store.test.ts | 15 + .../src/store-filesystem.ts | 4 +- 4 files changed, 491 insertions(+), 24 deletions(-) diff --git a/packages/host-kit/src/internal/process-lock.test.ts b/packages/host-kit/src/internal/process-lock.test.ts index 5e1d393147..aa14e9e955 100644 --- a/packages/host-kit/src/internal/process-lock.test.ts +++ b/packages/host-kit/src/internal/process-lock.test.ts @@ -15,6 +15,8 @@ import { acquireProcessLock, type ProcessLockOwner } from './process-lock.ts'; import { readProcessStartTime } from './host-process.ts'; import { mkdtempForTestSync } from './tmp-dir.fixtures.ts'; +const RECLAIMED_MARK = '.reclaimed-'; + let tmpDir: string; beforeEach(() => { @@ -142,6 +144,316 @@ test('acquireProcessLock reports live lock owner details on timeout', async () = ); }); +test('release leaves a lock whose record names a different process', async () => { + const lockDirPath = path.join(tmpDir, 'taken-over.lock'); + const ownerFilePath = path.join(lockDirPath, 'owner.json'); + const release = await acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + }); + + // A contender that reclaimed this lock while we were away republished the record + // with its own identity; removing the directory would give away its lock. + fs.writeFileSync( + ownerFilePath, + JSON.stringify({ pid: 999_999_999, startTime: null, acquiredAtMs: Date.now() }), + ); + await release(); + assert.equal(fs.existsSync(ownerFilePath), true); +}); + +test('acquireProcessLock does not evict a live owner whose owner.json is malformed', async () => { + const lockDirPath = path.join(tmpDir, 'malformed.lock'); + fs.mkdirSync(lockDirPath); + fs.writeFileSync(path.join(lockDirPath, 'owner.json'), '{ pid: '); + stampDirectoryAbandoned(lockDirPath); + + await assert.rejects( + () => + acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + timeoutMs: 50, + pollMs: 1, + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.ownerRecordUnreadable, true); + return true; + }, + ); + assert.equal(fs.existsSync(lockDirPath), true); +}); + +test('acquireProcessLock does not evict an owner record it cannot read', async () => { + const lockDirPath = path.join(tmpDir, 'unreadable.lock'); + fs.mkdirSync(lockDirPath); + // A directory where the record belongs fails the read with EISDIR rather than + // ENOENT, which is a live owner we know nothing about, not an unwritten one. + fs.mkdirSync(path.join(lockDirPath, 'owner.json')); + stampDirectoryAbandoned(lockDirPath); + + await assert.rejects( + () => + acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + timeoutMs: 50, + pollMs: 1, + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.ownerRecordUnreadable, true); + return true; + }, + ); + assert.equal(fs.existsSync(lockDirPath), true); +}); + +test('acquireProcessLock reclaims a lock whose owner record was never written', async () => { + const lockDirPath = path.join(tmpDir, 'unpublished.lock'); + fs.mkdirSync(lockDirPath); + stampDirectoryAbandoned(lockDirPath); + + const release = await acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + timeoutMs: 500, + pollMs: 1, + }); + await release(); + assert.equal(fs.existsSync(lockDirPath), false); +}); + +test('one abandoned lock offered to two contenders is held by exactly one of them', async () => { + const lockDirPath = path.join(tmpDir, 'contended.lock'); + fs.mkdirSync(lockDirPath); + stampDirectoryAbandoned(lockDirPath); + + const attempts = await Promise.allSettled([ + acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + ownerGraceMs: 0, + timeoutMs: 250, + pollMs: 2, + }), + acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + ownerGraceMs: 0, + timeoutMs: 250, + pollMs: 2, + }), + ]); + const acquired = attempts.filter((attempt) => attempt.status === 'fulfilled'); + const refused = attempts.filter((attempt) => attempt.status === 'rejected'); + + assert.equal(acquired.length, 1); + assert.equal(refused.length, 1); + const reason = (refused[0] as PromiseRejectedResult).reason; + assert.ok(reason instanceof AppError); + assert.equal(reason.details?.ownerLiveness, 'live'); + await (acquired[0] as PromiseFulfilledResult<() => Promise>).value(); + assert.deepEqual(listReclaimedSiblings(tmpDir), []); +}); + +function listReclaimedSiblings(directory: string): string[] { + return fs + .readdirSync(directory) + .filter((entry) => entry.includes(RECLAIMED_MARK)) + .sort(); +} + +const UNINFORMATIVE_OWNER_RECORDS = [ + 'null', + '"999999999"', + '{"pid":"999999999","startTime":null,"acquiredAtMs":1}', + '{"pid":0,"startTime":null,"acquiredAtMs":1}', + '{"pid":999999999,"startTime":7,"acquiredAtMs":1}', + '{"pid":999999999,"startTime":null}', +] as const; + +for (const [index, record] of UNINFORMATIVE_OWNER_RECORDS.entries()) { + test(`acquireProcessLock does not evict the lock behind the record ${record}`, async () => { + const lockDirPath = path.join(tmpDir, `uninformative-${index}.lock`); + fs.mkdirSync(lockDirPath); + fs.writeFileSync(path.join(lockDirPath, 'owner.json'), record); + stampDirectoryAbandoned(lockDirPath); + + await assert.rejects( + () => + acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + timeoutMs: 50, + pollMs: 1, + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.ownerRecordUnreadable, true); + return true; + }, + ); + assert.equal(fs.existsSync(lockDirPath), true); + }); +} + +test('release reports a lock whose owner record it cannot read instead of clearing it', async () => { + const lockDirPath = path.join(tmpDir, 'unverifiable-release.lock'); + const release = await acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + }); + fs.rmSync(path.join(lockDirPath, 'owner.json')); + fs.mkdirSync(path.join(lockDirPath, 'owner.json')); + + await assert.rejects( + () => release(), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.ownerReleaseUnverified, true); + assert.match(String(error.details?.hint), /unverifiable-release\.lock/); + return true; + }, + ); + assert.equal(fs.existsSync(lockDirPath), true); +}); + +test('acquireProcessLock reclaims a stray path in place of the lock directory', async () => { + const lockDirPath = path.join(tmpDir, 'stray.lock'); + fs.writeFileSync(lockDirPath, 'not a lock'); + const stale = new Date(Date.now() - 60_000); + fs.utimesSync(lockDirPath, stale, stale); + + const release = await acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + timeoutMs: 500, + pollMs: 1, + }); + await release(); + assert.equal(fs.existsSync(lockDirPath), false); +}); + +test('a forced reclaim leaves a directory that a live owner republished', async () => { + const lockDirPath = path.join(tmpDir, 'republished.lock'); + const ownerFilePath = path.join(lockDirPath, 'owner.json'); + fs.mkdirSync(lockDirPath); + fs.writeFileSync( + ownerFilePath, + JSON.stringify({ pid: 999_999_999, startTime: null, acquiredAtMs: Date.now() }), + ); + + // A win32 handle refuses the rename, and by the time the forced removal would run a + // live holder has claimed the path: the forced removal must not reach its directory. + const realRename = fs.renameSync; + const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation((( + from: fs.PathLike, + to: fs.PathLike, + ) => { + if (!String(to).includes(RECLAIMED_MARK)) return realRename(from, to); + fs.rmSync(String(from), { recursive: true, force: true }); + fs.mkdirSync(String(from)); + fs.writeFileSync(path.join(String(from), 'owner.json'), JSON.stringify(currentProcessOwner())); + throw Object.assign(new Error('operation not permitted'), { code: 'EPERM' }); + }) as typeof fs.renameSync); + + try { + await assert.rejects( + () => + acquireProcessLock({ + lockDirPath, + owner: { pid: 999_999_998, startTime: null, acquiredAtMs: Date.now() }, + timeoutMs: 50, + pollMs: 1, + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.ownerLiveness, 'live'); + return true; + }, + ); + assert.equal(fs.existsSync(ownerFilePath), true); + } finally { + renameSpy.mockRestore(); + } +}); + +test('a reclaim that cannot remove the directory it moved aside still holds the lock', async () => { + const lockDirPath = path.join(tmpDir, 'immovable.lock'); + fs.mkdirSync(lockDirPath); + fs.writeFileSync( + path.join(lockDirPath, 'owner.json'), + JSON.stringify({ pid: 999_999_999, startTime: null, acquiredAtMs: Date.now() }), + ); + stampDirectoryAbandoned(lockDirPath); + + const realRemove = fs.rmSync; + const removeSpy = vi.spyOn(fs, 'rmSync').mockImplementation(((target: fs.PathLike, options) => { + if (String(target).includes(RECLAIMED_MARK)) { + throw Object.assign(new Error('directory is busy'), { code: 'EBUSY' }); + } + return realRemove(target, options); + }) as typeof fs.rmSync); + + try { + const release = await acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + timeoutMs: 500, + pollMs: 1, + }); + assert.equal(fs.existsSync(path.join(lockDirPath, 'owner.json')), true); + await release(); + } finally { + removeSpy.mockRestore(); + } +}); + +test('a reclaim whose directory another contender moved aside retries and acquires', async () => { + const lockDirPath = path.join(tmpDir, 'lost-race.lock'); + fs.mkdirSync(lockDirPath); + fs.writeFileSync( + path.join(lockDirPath, 'owner.json'), + JSON.stringify({ pid: 999_999_999, startTime: null, acquiredAtMs: Date.now() }), + ); + stampDirectoryAbandoned(lockDirPath); + + // The contender that loses the rename finds the path already gone and cannot have + // cleared anything; it goes back to `mkdir`, which is what decides the lock. + let attempted = 0; + const realRename = fs.renameSync; + const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation((( + from: fs.PathLike, + to: fs.PathLike, + ) => { + if (!String(to).includes(RECLAIMED_MARK)) return realRename(from, to); + attempted += 1; + fs.rmSync(lockDirPath, { recursive: true, force: true }); + throw Object.assign(new Error('no such directory'), { code: 'ENOENT' }); + }) as typeof fs.renameSync); + + try { + const release = await acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + timeoutMs: 500, + pollMs: 1, + }); + assert.equal(attempted, 1); + assert.equal(fs.existsSync(path.join(lockDirPath, 'owner.json')), true); + await release(); + } finally { + renameSpy.mockRestore(); + } +}); + +function stampDirectoryAbandoned(directory: string): void { + const abandoned = new Date(Date.now() - 60_000); + fs.utimesSync(directory, abandoned, abandoned); +} + function currentProcessOwner(): ProcessLockOwner { return { pid: process.pid, diff --git a/packages/host-kit/src/internal/process-lock.ts b/packages/host-kit/src/internal/process-lock.ts index 7b9965ceea..d43bd6f9fe 100644 --- a/packages/host-kit/src/internal/process-lock.ts +++ b/packages/host-kit/src/internal/process-lock.ts @@ -1,13 +1,15 @@ +import crypto from 'node:crypto'; import fs from 'node:fs'; import path from 'node:path'; import { AppError } from '@agent-device/kernel/errors'; import { publishFileSync } from './atomic-file.ts'; -import { classifyOwnerLiveness } from './owner-identity.ts'; +import { classifyOwnerLiveness, ownerIdentityMatches } from './owner-identity.ts'; import { sleep } from './timeouts.ts'; const DEFAULT_LOCK_TIMEOUT_MS = 30_000; const DEFAULT_LOCK_POLL_MS = 100; const DEFAULT_LOCK_OWNER_GRACE_MS = 5_000; +const LOCK_DIRECTORY_SUFFIX = '.lock'; export type ProcessLockOwner = { pid: number; @@ -15,6 +17,11 @@ export type ProcessLockOwner = { acquiredAtMs: number; }; +type ProcessLockOwnerReading = + | { kind: 'owner'; owner: ProcessLockOwner } + | { kind: 'unwritten' } + | { kind: 'unreadable' }; + export async function acquireProcessLock(params: { lockDirPath: string; owner: ProcessLockOwner; @@ -39,8 +46,18 @@ export async function acquireProcessLock(params: { let released = false; return async () => { if (released) return; - released = true; - fs.rmSync(lockDirPath, { recursive: true, force: true }); + const outcome = releaseProcessLock(lockDirPath, ownerFilePath, owner); + if (outcome !== 'unverified') { + released = true; + return; + } + // The record still names us as far as we can tell and we could not read far + // enough to be sure, so the lock stays in place and the caller hears why. + throw new AppError('COMMAND_FAILED', `Cannot verify ownership of ${description}`, { + lockDirPath, + ownerReleaseUnverified: true, + hint: staleLockHint(lockDirPath), + }); }; } catch (error) { const err = error as NodeJS.ErrnoException; @@ -54,12 +71,18 @@ export async function acquireProcessLock(params: { } } + const reading = readProcessLockOwner(ownerFilePath); throw new AppError('COMMAND_FAILED', `Timed out waiting for ${description}`, { lockDirPath, - ...readProcessLockDiagnostics(lockDirPath, ownerFilePath), + ...readProcessLockDiagnostics(lockDirPath, reading), + ...(reading.kind === 'unreadable' ? { hint: staleLockHint(lockDirPath) } : {}), }); } +function staleLockHint(lockDirPath: string): string { + return `Remove ${lockDirPath} once you have confirmed no live process holds it, then retry.`; +} + function writeProcessLockOwner(ownerFilePath: string, owner: ProcessLockOwner): void { publishFileSync({ destination: ownerFilePath, @@ -67,61 +90,176 @@ function writeProcessLockOwner(ownerFilePath: string, owner: ProcessLockOwner): }); } +/** + * Removes the lock only while the record inside still names this acquirer. A lock + * that was reclaimed from under us belongs to whoever publishes there now, and + * deleting that directory would hand its holder's exclusion to a third contender. + */ +function releaseProcessLock( + lockDirPath: string, + ownerFilePath: string, + owner: ProcessLockOwner, +): 'removed' | 'not-owner' | 'unverified' { + const reading = readProcessLockOwner(ownerFilePath); + if (reading.kind === 'unreadable') return 'unverified'; + if (reading.kind === 'unwritten' || !ownerIdentityMatches(reading.owner, owner)) + return 'not-owner'; + fs.rmSync(lockDirPath, { recursive: true, force: true }); + return 'removed'; +} + function clearStaleProcessLock( lockDirPath: string, ownerFilePath: string, ownerGraceMs: number, ): boolean { - let ownerStats: fs.Stats | null = null; + let lockStats: fs.Stats; try { - ownerStats = fs.statSync(lockDirPath); + lockStats = fs.statSync(lockDirPath); } catch { return true; } - const owner = readProcessLockOwner(ownerFilePath); - if (owner) { - if (isLiveProcessLockOwner(owner)) { + // A lock path held by anything that is not a directory cannot carry a readable + // owner record, so its age is the only evidence available about it. + if (!lockStats.isDirectory()) { + return reclaimWhenAbandoned(lockStats, ownerGraceMs) + ? reclaimProcessLockDirectory(lockDirPath, ownerFilePath) + : false; + } + + const reading = readProcessLockOwner(ownerFilePath); + if (reading.kind === 'owner') { + if (isLiveProcessLockOwner(reading.owner)) { return false; } - fs.rmSync(lockDirPath, { recursive: true, force: true }); - return true; + return reclaimProcessLockDirectory(lockDirPath, ownerFilePath); } - if (Date.now() - ownerStats.mtimeMs < ownerGraceMs) { + // A record we cannot read leaves an owner whose identity is unknown, which is not + // evidence of death. Only a record that is genuinely absent lets the directory's + // own age speak for it. + if (reading.kind === 'unreadable') { return false; } - fs.rmSync(lockDirPath, { recursive: true, force: true }); + return reclaimWhenAbandoned(lockStats, ownerGraceMs) + ? reclaimProcessLockDirectory(lockDirPath, ownerFilePath) + : false; +} + +function reclaimWhenAbandoned(lockStats: fs.Stats, ownerGraceMs: number): boolean { + return Date.now() - lockStats.mtimeMs >= ownerGraceMs; +} + +/** + * Moves the abandoned directory aside under a unique name before removing it, so + * exactly one contender can reclaim one lock. Two plain removals look different from + * the caller's side: the second succeeds silently on the path the first already + * cleared, and both contenders continue as though they had freed the lock. `mkdir`'s + * `EEXIST`, unchanged, stays the arbiter of the lock itself. + * + * A win32 directory with a handle open inside it refuses the rename and often the + * removal too, which is why a forced removal remains as the fallback: stale-clear + * atomicity is best effort there and exact elsewhere. The fallback re-reads the + * record first, because a refused rename means time passed and a live contender may + * have claimed the path in it. + */ +function reclaimProcessLockDirectory(lockDirPath: string, ownerFilePath: string): boolean { + const asidePath = reclaimedLockPath(lockDirPath); + try { + fs.renameSync(lockDirPath, asidePath); + } catch (error) { + const code = (error as NodeJS.ErrnoException | null)?.code; + if (code === 'ENOENT') return true; + if (code !== 'EPERM' && code !== 'EACCES' && code !== 'ENOTEMPTY') return false; + const reading = readProcessLockOwner(ownerFilePath); + if (reading.kind === 'owner' && isLiveProcessLockOwner(reading.owner)) return false; + fs.rmSync(lockDirPath, { recursive: true, force: true }); + return true; + } + removeReclaimedLockDirectory(asidePath); return true; } -function readProcessLockOwner(ownerFilePath: string): ProcessLockOwner | null { +/** Janitorial work after the rename already achieved the exclusion. */ +function removeReclaimedLockDirectory(asidePath: string): void { + try { + fs.rmSync(asidePath, { recursive: true, force: true }); + } catch {} +} + +/** Keeps the `.lock` suffix so a sibling scanner still reads the name as a lock. */ +function reclaimedLockPath(lockDirPath: string): string { + const token = `${process.pid}-${crypto.randomUUID()}`; + const stem = lockDirPath.endsWith(LOCK_DIRECTORY_SUFFIX) + ? lockDirPath.slice(0, -LOCK_DIRECTORY_SUFFIX.length) + : lockDirPath; + return `${stem}.reclaimed-${token}${LOCK_DIRECTORY_SUFFIX}`; +} + +/** + * `ENOENT` is the only failure that means no record was written yet. Any other error, + * and any record that does not name a process, says a record exists that we cannot + * read, which is an owner of unknown liveness rather than an absent one. + */ +function readProcessLockOwner(ownerFilePath: string): ProcessLockOwnerReading { + let contents: string; try { - return JSON.parse(fs.readFileSync(ownerFilePath, 'utf8')) as ProcessLockOwner; + contents = fs.readFileSync(ownerFilePath, 'utf8'); + } catch (error) { + const code = (error as NodeJS.ErrnoException | null)?.code; + return code === 'ENOENT' ? { kind: 'unwritten' } : { kind: 'unreadable' }; + } + const owner = parseProcessLockOwner(contents); + return owner ? { kind: 'owner', owner } : { kind: 'unreadable' }; +} + +function parseProcessLockOwner(contents: string): ProcessLockOwner | null { + let parsed: unknown; + try { + parsed = JSON.parse(contents); } catch { return null; } + if (typeof parsed !== 'object' || parsed === null) return null; + const record = parsed as Record; + for (const [field, holdsShape] of Object.entries(PROCESS_LOCK_OWNER_FIELD_SHAPES)) { + if (!holdsShape(record[field])) return null; + } + return { + pid: record.pid as number, + startTime: typeof record.startTime === 'string' ? record.startTime : null, + acquiredAtMs: record.acquiredAtMs as number, + }; } +const PROCESS_LOCK_OWNER_FIELD_SHAPES: Record boolean> = + { + pid: (value) => typeof value === 'number' && Number.isInteger(value) && value > 0, + acquiredAtMs: (value) => typeof value === 'number' && Number.isFinite(value), + startTime: (value) => value === undefined || value === null || typeof value === 'string', + }; + function readProcessLockDiagnostics( lockDirPath: string, - ownerFilePath: string, + reading: ProcessLockOwnerReading, ): Record { const nowMs = Date.now(); - const owner = readProcessLockOwner(ownerFilePath); let lockAgeMs: number | undefined; try { lockAgeMs = Math.max(0, Math.round(nowMs - fs.statSync(lockDirPath).mtimeMs)); } catch {} return { ...(lockAgeMs !== undefined ? { lockAgeMs } : {}), - ...(owner + ...(reading.kind === 'owner' ? { - ownerPid: owner.pid, - ownerStartTime: owner.startTime, - ownerAgeMs: Math.max(0, Math.round(nowMs - owner.acquiredAtMs)), - ownerLiveness: classifyOwnerLiveness({ owner }), + ownerPid: reading.owner.pid, + ownerStartTime: reading.owner.startTime, + ownerAgeMs: Math.max(0, Math.round(nowMs - reading.owner.acquiredAtMs)), + ownerLiveness: classifyOwnerLiveness({ owner: reading.owner }), } - : {}), + : reading.kind === 'unreadable' + ? { ownerRecordUnreadable: true } + : {}), }; } diff --git a/packages/managed-allocation/src/__tests__/store.test.ts b/packages/managed-allocation/src/__tests__/store.test.ts index 6dd07be6e9..e02f11f59c 100644 --- a/packages/managed-allocation/src/__tests__/store.test.ts +++ b/packages/managed-allocation/src/__tests__/store.test.ts @@ -126,6 +126,21 @@ test('retains corrupt, unsupported, unfenced, and path-ambiguous records as diag assert.equal(path.dirname(path.dirname(operationPath)), path.join(root, 'allocations')); }); +test('listing a lane lock and the directory a stale reclaim renamed aside finds no extra record', () => { + const { root, store, record } = fixture(); + store.create(record); + const listed = store.list(); + const allocationsDir = path.join(root, 'allocations'); + const lane = path.basename(path.dirname(store.resolvePath(record))); + for (const lockName of [`${lane}.lane.lock`, `${lane}.reclaimed-4242-0.lock`]) { + const lockDir = path.join(allocationsDir, lockName); + fs.mkdirSync(lockDir, { recursive: true }); + fs.writeFileSync(path.join(lockDir, 'owner.json'), JSON.stringify({ pid: process.pid })); + } + + assert.deepEqual(store.list(), listed); +}); + test('does not follow a symbolic-link destination', () => { const { root, store, record } = fixture(); const operationPath = store.resolvePath(record); diff --git a/packages/managed-allocation/src/store-filesystem.ts b/packages/managed-allocation/src/store-filesystem.ts index 191981c19e..425702d2cf 100644 --- a/packages/managed-allocation/src/store-filesystem.ts +++ b/packages/managed-allocation/src/store-filesystem.ts @@ -114,7 +114,9 @@ function readDirectory(directory: string): DirectoryRead { } function listLanePaths(allocationsDir: string, lane: fs.Dirent): AllocationOperationPath[] { - if (lane.name.endsWith('.lane.lock')) return []; + // Lane and operation locks sit beside the records, and a stale reclaim renames one + // aside under its own name; none of those directories holds an operation record. + if (lane.name.endsWith('.lock')) return []; const lanePath = path.join(allocationsDir, lane.name); if (!lane.isDirectory()) { return [ From 5d3444f5779fbdf0dec3f923a2c76a7bfa3db7f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 14 Sep 2026 18:07:53 +0200 Subject: [PATCH 02/18] fix(host-kit): a reclaimed lock is proven to be the one judged stale before removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rename addresses whatever stands at the lock path now, not the directory whose record was read. A contender that reclaimed first and published a live owner in the meantime had its live directory moved aside by the loser and then removed, which handed the same lock to two holders. What arrives is now compared against what was judged — same inode, and no live owner inside — and a directory that turns out to belong to someone else is renamed back rather than deleted. `releaseProcessLock` can now refuse to clear a lock it cannot prove it owns, so the callers that run a task under a lock stop letting that verdict displace the task's own failure: the unverifiable release is reported when the task succeeded, and suppressed when the task already failed, because the lock's stale-clear path resolves a lock that is still standing while nothing else recovers why the task failed. The abandoned cache-lock acquire releases without leaving a rejected promise nobody awaits. Co-authored-by: Apex by Callstack --- .../capture-kit/src/recording/swift-cache.ts | 4 +- .../src/internal/process-lock.test.ts | 61 +++++++++++++ .../host-kit/src/internal/process-lock.ts | 82 +++++++++++++---- .../runner-lease-release-ownership.test.ts | 87 +++++++++++++++++++ .../src/runner/runner-device-set.ts | 4 +- .../platform-apple/src/runner/runner-lease.ts | 9 +- .../src/snapshot-source/host.ts | 9 +- .../platform-web/src/agent-browser-tool.ts | 9 +- src/daemon/device-claim-store.ts | 9 +- 9 files changed, 246 insertions(+), 28 deletions(-) create mode 100644 packages/platform-apple/src/runner/__tests__/runner-lease-release-ownership.test.ts diff --git a/packages/capture-kit/src/recording/swift-cache.ts b/packages/capture-kit/src/recording/swift-cache.ts index ad382f5a8b..a709e3a999 100644 --- a/packages/capture-kit/src/recording/swift-cache.ts +++ b/packages/capture-kit/src/recording/swift-cache.ts @@ -128,7 +128,9 @@ async function ensureSwiftExecutable(params: { fs.renameSync(tempExecutablePath, params.executablePath); } finally { fs.rmSync(tempDir, { recursive: true, force: true }); - await releaseLock(); + // The build's own failure is the reportable fact; an unverified release leaves the + // lock to the stale-clear path rather than displacing it. + await releaseLock().catch(() => undefined); } } diff --git a/packages/host-kit/src/internal/process-lock.test.ts b/packages/host-kit/src/internal/process-lock.test.ts index aa14e9e955..cab1713d1c 100644 --- a/packages/host-kit/src/internal/process-lock.test.ts +++ b/packages/host-kit/src/internal/process-lock.test.ts @@ -411,6 +411,67 @@ test('a reclaim that cannot remove the directory it moved aside still holds the } }); +test('a live owner published between the stale read and the rename keeps its lock', async () => { + const lockDirPath = path.join(tmpDir, 'stolen-race.lock'); + const ownerFilePath = path.join(lockDirPath, 'owner.json'); + fs.mkdirSync(lockDirPath); + fs.writeFileSync( + ownerFilePath, + JSON.stringify({ pid: 999_999_999, startTime: null, acquiredAtMs: Date.now() }), + ); + stampDirectoryAbandoned(lockDirPath); + + // The dead record is read, and before the rename lands another contender reclaims the + // path, publishes itself, and goes live. The rename then moves that live directory, and + // the only thing that can tell it apart from the one judged abandoned is the directory + // itself. + let republished = false; + const realRename = fs.renameSync; + const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation((( + from: fs.PathLike, + to: fs.PathLike, + ) => { + if (!String(to).includes(RECLAIMED_MARK) || republished) return realRename(from, to); + republished = true; + fs.rmSync(String(from), { recursive: true, force: true }); + fs.mkdirSync(String(from)); + fs.writeFileSync(path.join(String(from), 'owner.json'), JSON.stringify(currentProcessOwner())); + return realRename(from, to); + }) as typeof fs.renameSync); + + try { + await assert.rejects( + () => + acquireProcessLock({ + lockDirPath, + owner: { pid: 999_999_998, startTime: null, acquiredAtMs: Date.now() }, + timeoutMs: 50, + pollMs: 1, + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.ownerLiveness, 'live'); + assert.equal(error.details?.ownerPid, process.pid); + return true; + }, + ); + assert.equal(republished, true); + assert.equal( + (JSON.parse(fs.readFileSync(ownerFilePath, 'utf8')) as { pid: number }).pid, + process.pid, + ); + assert.deepEqual( + fs + .readdirSync(tmpDir) + .filter((name) => name.includes(RECLAIMED_MARK)) + .sort(), + [], + ); + } finally { + renameSpy.mockRestore(); + } +}); + test('a reclaim whose directory another contender moved aside retries and acquires', async () => { const lockDirPath = path.join(tmpDir, 'lost-race.lock'); fs.mkdirSync(lockDirPath); diff --git a/packages/host-kit/src/internal/process-lock.ts b/packages/host-kit/src/internal/process-lock.ts index d43bd6f9fe..f531d18d2f 100644 --- a/packages/host-kit/src/internal/process-lock.ts +++ b/packages/host-kit/src/internal/process-lock.ts @@ -6,6 +6,7 @@ import { publishFileSync } from './atomic-file.ts'; import { classifyOwnerLiveness, ownerIdentityMatches } from './owner-identity.ts'; import { sleep } from './timeouts.ts'; +const OWNER_FILE_NAME = 'owner.json'; const DEFAULT_LOCK_TIMEOUT_MS = 30_000; const DEFAULT_LOCK_POLL_MS = 100; const DEFAULT_LOCK_OWNER_GRACE_MS = 5_000; @@ -31,7 +32,7 @@ export async function acquireProcessLock(params: { description?: string; }): Promise<() => Promise> { const { lockDirPath, owner } = params; - const ownerFilePath = path.join(lockDirPath, 'owner.json'); + const ownerFilePath = path.join(lockDirPath, OWNER_FILE_NAME); const deadline = Date.now() + (params.timeoutMs ?? DEFAULT_LOCK_TIMEOUT_MS); const pollMs = params.pollMs ?? DEFAULT_LOCK_POLL_MS; const ownerGraceMs = params.ownerGraceMs ?? DEFAULT_LOCK_OWNER_GRACE_MS; @@ -124,7 +125,7 @@ function clearStaleProcessLock( // owner record, so its age is the only evidence available about it. if (!lockStats.isDirectory()) { return reclaimWhenAbandoned(lockStats, ownerGraceMs) - ? reclaimProcessLockDirectory(lockDirPath, ownerFilePath) + ? reclaimProcessLockDirectory(lockDirPath, ownerFilePath, lockStats) : false; } @@ -133,7 +134,7 @@ function clearStaleProcessLock( if (isLiveProcessLockOwner(reading.owner)) { return false; } - return reclaimProcessLockDirectory(lockDirPath, ownerFilePath); + return reclaimProcessLockDirectory(lockDirPath, ownerFilePath, lockStats); } // A record we cannot read leaves an owner whose identity is unknown, which is not // evidence of death. Only a record that is genuinely absent lets the directory's @@ -142,7 +143,7 @@ function clearStaleProcessLock( return false; } return reclaimWhenAbandoned(lockStats, ownerGraceMs) - ? reclaimProcessLockDirectory(lockDirPath, ownerFilePath) + ? reclaimProcessLockDirectory(lockDirPath, ownerFilePath, lockStats) : false; } @@ -156,30 +157,75 @@ function reclaimWhenAbandoned(lockStats: fs.Stats, ownerGraceMs: number): boolea * the caller's side: the second succeeds silently on the path the first already * cleared, and both contenders continue as though they had freed the lock. `mkdir`'s * `EEXIST`, unchanged, stays the arbiter of the lock itself. - * - * A win32 directory with a handle open inside it refuses the rename and often the - * removal too, which is why a forced removal remains as the fallback: stale-clear - * atomicity is best effort there and exact elsewhere. The fallback re-reads the - * record first, because a refused rename means time passed and a live contender may - * have claimed the path in it. */ -function reclaimProcessLockDirectory(lockDirPath: string, ownerFilePath: string): boolean { +function reclaimProcessLockDirectory( + lockDirPath: string, + ownerFilePath: string, + judged: fs.Stats, +): boolean { const asidePath = reclaimedLockPath(lockDirPath); try { fs.renameSync(lockDirPath, asidePath); } catch (error) { - const code = (error as NodeJS.ErrnoException | null)?.code; - if (code === 'ENOENT') return true; - if (code !== 'EPERM' && code !== 'EACCES' && code !== 'ENOTEMPTY') return false; - const reading = readProcessLockOwner(ownerFilePath); - if (reading.kind === 'owner' && isLiveProcessLockOwner(reading.owner)) return false; - fs.rmSync(lockDirPath, { recursive: true, force: true }); - return true; + return reclaimWithoutTheRename(lockDirPath, ownerFilePath, error); + } + if (!reclaimedLockIsTheOneJudged(asidePath, judged)) { + restoreReclaimedLockDirectory(asidePath, lockDirPath); + return false; } removeReclaimedLockDirectory(asidePath); return true; } +/** + * A win32 directory with a handle open inside it refuses the rename and often the removal + * too, which is why a forced removal remains as the fallback: stale-clear atomicity is + * best effort there and exact elsewhere. The fallback re-reads the record first, because a + * refused rename means time passed and a live contender may have claimed the path in it. + */ +function reclaimWithoutTheRename( + lockDirPath: string, + ownerFilePath: string, + renameError: unknown, +): boolean { + const code = (renameError as NodeJS.ErrnoException | null)?.code; + if (code === 'ENOENT') return true; + if (code !== 'EPERM' && code !== 'EACCES' && code !== 'ENOTEMPTY') return false; + const reading = readProcessLockOwner(ownerFilePath); + if (reading.kind === 'owner' && isLiveProcessLockOwner(reading.owner)) return false; + fs.rmSync(lockDirPath, { recursive: true, force: true }); + return true; +} + +/** + * A rename addresses whatever stands at the path now, not the directory whose record was + * read. A contender that reclaimed first and published a live owner in the meantime has + * put a different directory there, so what arrived is compared against what was judged: + * the same inode, and no live owner inside. + */ +function reclaimedLockIsTheOneJudged(asidePath: string, judged: fs.Stats): boolean { + let moved: fs.Stats; + try { + moved = fs.statSync(asidePath); + } catch { + return false; + } + if (moved.ino !== judged.ino || moved.dev !== judged.dev) return false; + const reading = readProcessLockOwner(path.join(asidePath, OWNER_FILE_NAME)); + return !(reading.kind === 'owner' && isLiveProcessLockOwner(reading.owner)); +} + +/** + * Returns a directory that turned out to belong to someone else. Failure is not a + * licence to delete it: its holder's own release reports an unreadable owner rather than + * let this process clear a lock it does not own. + */ +function restoreReclaimedLockDirectory(asidePath: string, lockDirPath: string): void { + try { + fs.renameSync(asidePath, lockDirPath); + } catch {} +} + /** Janitorial work after the rename already achieved the exclusion. */ function removeReclaimedLockDirectory(asidePath: string): void { try { diff --git a/packages/platform-apple/src/runner/__tests__/runner-lease-release-ownership.test.ts b/packages/platform-apple/src/runner/__tests__/runner-lease-release-ownership.test.ts new file mode 100644 index 0000000000..ec82736adb --- /dev/null +++ b/packages/platform-apple/src/runner/__tests__/runner-lease-release-ownership.test.ts @@ -0,0 +1,87 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import { afterEach, test, vi } from 'vitest'; +import { AppError } from '@agent-device/kernel/errors'; +import { withRunnerLeaseLock } from '../runner-lease.ts'; +import { mkdtempForTestSync } from './tmp-dir.ts'; + +// A release that cannot read far enough to prove ownership leaves the lock standing and +// says so. That verdict is worth reporting when the task succeeded, and worth suppressing +// when the task already failed: the lock's own stale-clear path resolves a lock that is +// still standing, while nothing else recovers the reason the task failed. + +const DEVICE_ID = 'release-ownership-runner'; + +let leaseRoot = ''; + +afterEach(() => { + vi.restoreAllMocks(); + delete process.env.AGENT_DEVICE_IOS_RUNNER_LEASE_DIR; + fs.rmSync(leaseRoot, { recursive: true, force: true }); +}); + +function armUnverifiableRelease(root: string): () => void { + const originalReadFileSync = fs.readFileSync; + let armed = false; + vi.spyOn(fs, 'readFileSync').mockImplementation((( + ...args: Parameters + ) => { + const target = args[0]; + if ( + armed && + typeof target === 'string' && + target.startsWith(root) && + target.endsWith('owner.json') + ) { + throw Object.assign(new Error('injected owner record i/o'), { code: 'EIO' }); + } + return Reflect.apply(originalReadFileSync, fs, args); + }) as typeof fs.readFileSync); + return () => { + armed = true; + }; +} + +function beginLeaseRoot(): string { + leaseRoot = mkdtempForTestSync('agent-device-lease-release-ownership-'); + process.env.AGENT_DEVICE_IOS_RUNNER_LEASE_DIR = leaseRoot; + return leaseRoot; +} + +test('a failed lease task outranks an unverifiable release', async () => { + const root = beginLeaseRoot(); + const arm = armUnverifiableRelease(root); + const taskFailure = new Error('the lease task failed'); + + await assert.rejects( + () => + withRunnerLeaseLock(DEVICE_ID, async () => { + arm(); + throw taskFailure; + }), + (error: unknown) => { + assert.equal(error, taskFailure); + return true; + }, + ); +}); + +test('a lease lock that cannot be verified at release is reported and left standing', async () => { + const root = beginLeaseRoot(); + const arm = armUnverifiableRelease(root); + + await assert.rejects( + () => + withRunnerLeaseLock(DEVICE_ID, async () => { + arm(); + return 'done'; + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.ownerReleaseUnverified, true); + return true; + }, + ); + + assert.deepEqual(fs.readdirSync(root), [`${DEVICE_ID}.json.lock`]); +}); diff --git a/packages/platform-apple/src/runner/runner-device-set.ts b/packages/platform-apple/src/runner/runner-device-set.ts index f9d8176b88..ab95ce021a 100644 --- a/packages/platform-apple/src/runner/runner-device-set.ts +++ b/packages/platform-apple/src/runner/runner-device-set.ts @@ -99,7 +99,9 @@ export async function acquireXcodebuildSimulatorSetRedirect( xctestDeviceSetPath, backupPath, }); - await releaseLock(); + // The redirect failure is the reportable fact; an unverified release leaves the lock + // to the stale-clear path rather than displacing it. + await releaseLock().catch(() => undefined); throw new AppError('COMMAND_FAILED', 'Failed to redirect XCTest device set path', { requestedSetPath, xctestDeviceSetPath, diff --git a/packages/platform-apple/src/runner/runner-lease.ts b/packages/platform-apple/src/runner/runner-lease.ts index a5bf3a3438..082df145d4 100644 --- a/packages/platform-apple/src/runner/runner-lease.ts +++ b/packages/platform-apple/src/runner/runner-lease.ts @@ -128,9 +128,14 @@ export async function withRunnerLeaseLock(deviceId: string, task: () => Promi description: `iOS runner lease for ${deviceId}`, }); try { - return await task(); - } finally { + const result = await task(); await release(); + return result; + } catch (error) { + // A task that failed is the reportable fact; an unverified release only says the + // lease lock is still standing, which the stale-clear path resolves on its own. + await release().catch(() => undefined); + throw error; } } diff --git a/packages/platform-apple/src/snapshot-source/host.ts b/packages/platform-apple/src/snapshot-source/host.ts index 40aed99550..d658ec3478 100644 --- a/packages/platform-apple/src/snapshot-source/host.ts +++ b/packages/platform-apple/src/snapshot-source/host.ts @@ -195,11 +195,16 @@ async function acquireSnapshotSourceLock( try { return await Promise.race([pending, aborted]); } catch (error) { - if (canceled) + if (canceled) { + // The task is abandoned, so its lock is released best effort. A release that + // cannot prove ownership leaves the lock to the stale-clear path, which is the + // outcome this branch already accepts; it must not arrive as an unhandled + // rejection on a promise nobody is awaiting any more. void pending.then( - (release) => release(), + (release) => release().catch(() => undefined), () => undefined, ); + } if ( deadline.clock.isExpired() && !(error instanceof SnapshotSourceError && error.failureKind === 'cancelled') diff --git a/packages/platform-web/src/agent-browser-tool.ts b/packages/platform-web/src/agent-browser-tool.ts index 29c419fdba..01eaed5ffe 100644 --- a/packages/platform-web/src/agent-browser-tool.ts +++ b/packages/platform-web/src/agent-browser-tool.ts @@ -101,9 +101,14 @@ export async function setupManagedAgentBrowser(options: { packageName: AGENT_BROWSER, version: MANAGED_AGENT_BROWSER_VERSION, }); - return getManagedAgentBrowserStatus(options); - } finally { + const status = await getManagedAgentBrowserStatus(options); await release(); + return status; + } catch (error) { + // A failed install is the reportable fact; an unverified release leaves the lock to + // the stale-clear path. + await release().catch(() => undefined); + throw error; } } diff --git a/src/daemon/device-claim-store.ts b/src/daemon/device-claim-store.ts index 026cd51d27..53d5dac240 100644 --- a/src/daemon/device-claim-store.ts +++ b/src/daemon/device-claim-store.ts @@ -32,8 +32,13 @@ export async function withDeviceClaimLock( description: `device claim for ${deviceKey}`, }); try { - return await task(); - } finally { + const result = await task(); await release(); + return result; + } catch (error) { + // A task that failed is the reportable fact; an unverified release only says the + // lock is still standing, which the stale-clear path resolves on its own. + await release().catch(() => undefined); + throw error; } } From 47f58afec75c7e6838d839847e5c4104503ebb9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 14 Sep 2026 18:20:11 +0200 Subject: [PATCH 03/18] fix(host-kit): a lock claim is identified by its token, not only by its process Two records can name the same process and still be different acquisitions of the same path, which is the distinction a release and a reclaim both need. Each acquisition publishes a random claim token with its owner record: release matches the token rather than pid and start time alone, and a reclaim compares the token of the record inside the directory it moved with the one it judged before renaming. The publication-ownership rule now says what it means for the process lock: its owner record goes through the shared publication owner and it writes no file by hand, while its renames are addressed only between the lock path and the reclaimed name. Reclaiming a lock directory is a different claim of ownership from publishing a file into one. Co-authored-by: Apex by Callstack --- .../src/internal/process-lock.test.ts | 38 ++++++++++ .../host-kit/src/internal/process-lock.ts | 69 ++++++++++++++----- .../atomic-publish-ownership.test.ts | 20 +++++- 3 files changed, 109 insertions(+), 18 deletions(-) diff --git a/packages/host-kit/src/internal/process-lock.test.ts b/packages/host-kit/src/internal/process-lock.test.ts index cab1713d1c..3dc76b7bff 100644 --- a/packages/host-kit/src/internal/process-lock.test.ts +++ b/packages/host-kit/src/internal/process-lock.test.ts @@ -162,6 +162,44 @@ test('release leaves a lock whose record names a different process', async () => assert.equal(fs.existsSync(ownerFilePath), true); }); +test('release leaves a lock that a new acquisition of the same process republished', async () => { + const lockDirPath = path.join(tmpDir, 'reacquired.lock'); + const ownerFilePath = path.join(lockDirPath, 'owner.json'); + const owner = currentProcessOwner(); + const release = await acquireProcessLock({ lockDirPath, owner }); + + // Same pid, same start time: the only thing that can tell this record from ours is the + // claim written with it. Removing the directory would hand the new holder's lock away. + fs.writeFileSync( + ownerFilePath, + JSON.stringify({ ...owner, acquiredAtMs: Date.now(), claimToken: 'a-different-claim' }), + ); + await release(); + + assert.equal( + (JSON.parse(fs.readFileSync(ownerFilePath, 'utf8')) as { claimToken: string }).claimToken, + 'a-different-claim', + ); +}); + +test('a reacquired lock publishes a claim that its predecessor cannot reuse', async () => { + const lockDirPath = path.join(tmpDir, 'claim-token.lock'); + const ownerFilePath = path.join(lockDirPath, 'owner.json'); + const first = await acquireProcessLock({ lockDirPath, owner: currentProcessOwner() }); + const firstToken = (JSON.parse(fs.readFileSync(ownerFilePath, 'utf8')) as { claimToken: string }) + .claimToken; + await first(); + + const second = await acquireProcessLock({ lockDirPath, owner: currentProcessOwner() }); + const secondToken = (JSON.parse(fs.readFileSync(ownerFilePath, 'utf8')) as { claimToken: string }) + .claimToken; + await second(); + + assert.equal(typeof firstToken, 'string'); + assert.equal(typeof secondToken, 'string'); + assert.notEqual(firstToken, secondToken); +}); + test('acquireProcessLock does not evict a live owner whose owner.json is malformed', async () => { const lockDirPath = path.join(tmpDir, 'malformed.lock'); fs.mkdirSync(lockDirPath); diff --git a/packages/host-kit/src/internal/process-lock.ts b/packages/host-kit/src/internal/process-lock.ts index f531d18d2f..247a6a2c6f 100644 --- a/packages/host-kit/src/internal/process-lock.ts +++ b/packages/host-kit/src/internal/process-lock.ts @@ -18,8 +18,17 @@ export type ProcessLockOwner = { acquiredAtMs: number; }; +/** + * One acquisition of a lock. The token says which: two records can name the same process + * and still be different claims on the same path, which is what a release and a reclaim + * have to tell apart. + */ +export type ProcessLockOwnerRecord = ProcessLockOwner & { + claimToken: string | null; +}; + type ProcessLockOwnerReading = - | { kind: 'owner'; owner: ProcessLockOwner } + | { kind: 'owner'; owner: ProcessLockOwnerRecord } | { kind: 'unwritten' } | { kind: 'unreadable' }; @@ -39,15 +48,16 @@ export async function acquireProcessLock(params: { const description = params.description ?? 'process lock'; fs.mkdirSync(path.dirname(lockDirPath), { recursive: true }); + const claim: ProcessLockOwnerRecord = { ...owner, claimToken: crypto.randomUUID() }; while (Date.now() < deadline) { try { fs.mkdirSync(lockDirPath); - writeProcessLockOwner(ownerFilePath, owner); + writeProcessLockOwner(ownerFilePath, claim); let released = false; return async () => { if (released) return; - const outcome = releaseProcessLock(lockDirPath, ownerFilePath, owner); + const outcome = releaseProcessLock(lockDirPath, ownerFilePath, claim); if (outcome !== 'unverified') { released = true; return; @@ -84,7 +94,7 @@ function staleLockHint(lockDirPath: string): string { return `Remove ${lockDirPath} once you have confirmed no live process holds it, then retry.`; } -function writeProcessLockOwner(ownerFilePath: string, owner: ProcessLockOwner): void { +function writeProcessLockOwner(ownerFilePath: string, owner: ProcessLockOwnerRecord): void { publishFileSync({ destination: ownerFilePath, contents: JSON.stringify(owner), @@ -99,12 +109,15 @@ function writeProcessLockOwner(ownerFilePath: string, owner: ProcessLockOwner): function releaseProcessLock( lockDirPath: string, ownerFilePath: string, - owner: ProcessLockOwner, + claim: ProcessLockOwnerRecord, ): 'removed' | 'not-owner' | 'unverified' { const reading = readProcessLockOwner(ownerFilePath); if (reading.kind === 'unreadable') return 'unverified'; - if (reading.kind === 'unwritten' || !ownerIdentityMatches(reading.owner, owner)) + if (reading.kind === 'unwritten' || !ownerIdentityMatches(reading.owner, claim)) return 'not-owner'; + // The same process can hold this path twice in sequence, and a reclaim that moved our + // directory aside leaves a record behind that names us as though nothing had happened. + if (reading.owner.claimToken !== claim.claimToken) return 'not-owner'; fs.rmSync(lockDirPath, { recursive: true, force: true }); return 'removed'; } @@ -125,7 +138,10 @@ function clearStaleProcessLock( // owner record, so its age is the only evidence available about it. if (!lockStats.isDirectory()) { return reclaimWhenAbandoned(lockStats, ownerGraceMs) - ? reclaimProcessLockDirectory(lockDirPath, ownerFilePath, lockStats) + ? reclaimProcessLockDirectory(lockDirPath, ownerFilePath, { + stats: lockStats, + claimToken: null, + }) : false; } @@ -134,7 +150,10 @@ function clearStaleProcessLock( if (isLiveProcessLockOwner(reading.owner)) { return false; } - return reclaimProcessLockDirectory(lockDirPath, ownerFilePath, lockStats); + return reclaimProcessLockDirectory(lockDirPath, ownerFilePath, { + stats: lockStats, + claimToken: reading.owner.claimToken, + }); } // A record we cannot read leaves an owner whose identity is unknown, which is not // evidence of death. Only a record that is genuinely absent lets the directory's @@ -143,7 +162,10 @@ function clearStaleProcessLock( return false; } return reclaimWhenAbandoned(lockStats, ownerGraceMs) - ? reclaimProcessLockDirectory(lockDirPath, ownerFilePath, lockStats) + ? reclaimProcessLockDirectory(lockDirPath, ownerFilePath, { + stats: lockStats, + claimToken: null, + }) : false; } @@ -161,7 +183,7 @@ function reclaimWhenAbandoned(lockStats: fs.Stats, ownerGraceMs: number): boolea function reclaimProcessLockDirectory( lockDirPath: string, ownerFilePath: string, - judged: fs.Stats, + judged: JudgedLock, ): boolean { const asidePath = reclaimedLockPath(lockDirPath); try { @@ -199,22 +221,28 @@ function reclaimWithoutTheRename( /** * A rename addresses whatever stands at the path now, not the directory whose record was - * read. A contender that reclaimed first and published a live owner in the meantime has - * put a different directory there, so what arrived is compared against what was judged: - * the same inode, and no live owner inside. + * read. A contender that reclaimed first and claimed the path again has put a different + * directory there, so what arrived is compared against what was judged: the same inode, + * no live owner inside, and the claim token that was read before the rename. */ -function reclaimedLockIsTheOneJudged(asidePath: string, judged: fs.Stats): boolean { +function reclaimedLockIsTheOneJudged(asidePath: string, judged: JudgedLock): boolean { let moved: fs.Stats; try { moved = fs.statSync(asidePath); } catch { return false; } - if (moved.ino !== judged.ino || moved.dev !== judged.dev) return false; + if (moved.ino !== judged.stats.ino || moved.dev !== judged.stats.dev) return false; const reading = readProcessLockOwner(path.join(asidePath, OWNER_FILE_NAME)); - return !(reading.kind === 'owner' && isLiveProcessLockOwner(reading.owner)); + if (reading.kind === 'owner' && isLiveProcessLockOwner(reading.owner)) return false; + return readClaimToken(reading) === judged.claimToken; } +type JudgedLock = { + stats: fs.Stats; + claimToken: string | null; +}; + /** * Returns a directory that turned out to belong to someone else. Failure is not a * licence to delete it: its holder's own release reports an unreadable owner rather than @@ -259,7 +287,11 @@ function readProcessLockOwner(ownerFilePath: string): ProcessLockOwnerReading { return owner ? { kind: 'owner', owner } : { kind: 'unreadable' }; } -function parseProcessLockOwner(contents: string): ProcessLockOwner | null { +function readClaimToken(reading: ProcessLockOwnerReading): string | null { + return reading.kind === 'owner' ? reading.owner.claimToken : null; +} + +function parseProcessLockOwner(contents: string): ProcessLockOwnerRecord | null { let parsed: unknown; try { parsed = JSON.parse(contents); @@ -275,6 +307,9 @@ function parseProcessLockOwner(contents: string): ProcessLockOwner | null { pid: record.pid as number, startTime: typeof record.startTime === 'string' ? record.startTime : null, acquiredAtMs: record.acquiredAtMs as number, + // A record written before claims were tokenized names a process without saying which + // acquisition it was, which no release can match and no reclaim can be blamed for. + claimToken: typeof record.claimToken === 'string' ? record.claimToken : null, }; } diff --git a/src/daemon/__tests__/atomic-publish-ownership.test.ts b/src/daemon/__tests__/atomic-publish-ownership.test.ts index d86f3d07b2..804f93ba9f 100644 --- a/src/daemon/__tests__/atomic-publish-ownership.test.ts +++ b/src/daemon/__tests__/atomic-publish-ownership.test.ts @@ -11,9 +11,13 @@ const SIMPLE_PUBLISHERS = [ new URL('../session-script-writer.ts', import.meta.url), new URL('../../../packages/platform-apple/src/runner/runner-lease.ts', import.meta.url), new URL('../../remote/remote-connection-state.ts', import.meta.url), - new URL('../../../packages/host-kit/src/internal/process-lock.ts', import.meta.url), ] as const; +const PROCESS_LOCK_SOURCE = new URL( + '../../../packages/host-kit/src/internal/process-lock.ts', + import.meta.url, +); + test('simple same-directory publishers use the shared atomic publish owner', () => { for (const sourcePath of SIMPLE_PUBLISHERS) { const source = fs.readFileSync(sourcePath, 'utf8'); @@ -22,6 +26,20 @@ test('simple same-directory publishers use the shared atomic publish owner', () } }); +// The process lock publishes a file and reclaims a directory, which are two different +// claims of ownership: only the first belongs to the publication owners above. +test('the process lock publishes its owner record without publishing files by hand', () => { + const source = fs.readFileSync(PROCESS_LOCK_SOURCE, 'utf8'); + assert.match(source, /publishFileSync/); + assert.doesNotMatch(source, /fs\.writeFileSync\s*\(/); +}); + +test('the process lock renames only between the lock path and its reclaimed name', () => { + const source = fs.readFileSync(PROCESS_LOCK_SOURCE, 'utf8'); + const renamed = [...source.matchAll(/fs\.renameSync\(([^)]*)\)/g)].map((match) => match[1]); + assert.deepEqual(renamed.sort(), ['asidePath, lockDirPath', 'lockDirPath, asidePath']); +}); + test('durable publishers share the host-kit durable publication owner', () => { const sourcePaths = [ new URL('../../../packages/capture-kit/src/durable-capture/store.ts', import.meta.url), From 6c8c7c7dd4fada84c4bbac2816b8c603c02182ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 14 Sep 2026 20:33:48 +0200 Subject: [PATCH 04/18] fix(host-kit): a reclaim re-decides in place, and a release never speaks over its caller Rename-aside made a reclaim a two-step transaction: move the abandoned directory out from under the lock path, judge it there, and move it back if it turned out to belong to somebody else. Between those steps the lock path is simply absent, and a contender polling the path reads that as free: it claims the path and publishes, and the rename back returns `ENOTEMPTY` into a `catch {}`. The judged directory ends up nobody's. Removal happens in place now, behind `mkdir .reclaim`, and every branch decides again from what is on disk rather than from a `Stats` read before the mutex was taken: - a directory whose record named a dead claim goes only while that record is still there answering to the same claim token. A token is a random id no later acquisition repeats, so a contender that claimed the path in between walks away holding its lock; - a directory with no record has no claim to attribute its contents to, so `rmdir` is the only call made on it and its age is asked again. `ENOTEMPTY` is a publication saying so, and a directory dated a moment ago is an acquisition that has not published yet, not an abandoned one; - a path that is not a directory is unlinked, and `unlink` itself answers `EISDIR` for the one case this branch must not touch; - a path that has already gone is left alone, so the caller's next `mkdir` simply wins it. `releaseProcessLock` lost its recursive removal for the same reason: it unlinks the record it verified and removes the directory only while empty. Callers that were answering "which of two failures do I report?" by hand with `finally` now go through one shape: `withProcessLock({ acquire, task })` releases best-effort when the task failed and strictly when it did not. `runner-artifact.ts`, `runner-cache.ts`, `managed-allocation/src/store.ts` and `store-lock.ts` each had a release that could speak `ownerReleaseUnverified` over the task's own failure. `managed agent-browser setup gives the lock back on every path out` pins a second bug that shape was hiding: setup returned early when the package was already installed, skipping the only `await release()` and leaving the lock for the stale-clear path to notice five seconds later. The XCTest device-set redirect had that shape twice more, and neither site is a task: a launch that failed waits on the redirect in `runner-session.ts`, and a teardown in `runner-disposal.ts`. Both call `releaseXcodebuildSimulatorSetRedirectBestEffort` now, and the build path calls `withXcodebuildSimulatorSetRedirect`, so the `xcodebuild` failure is the error a caller reads. The parked directory is gone from the vocabulary as well: a stale reclaim holds `.reclaim.lock`, which is what the `managed-allocation` store comment and its lock-scan test now say, and the two-contender test asks with a second of grace rather than none, because a zero grace reads the winner's own mutex as abandoned. --- packages/host-kit/src/file.ts | 7 +- .../src/internal/process-lock.test.ts | 404 +++++++++++++----- .../host-kit/src/internal/process-lock.ts | 264 ++++++++---- .../src/__tests__/store.test.ts | 4 +- .../src/store-filesystem.ts | 4 +- packages/managed-allocation/src/store-lock.ts | 19 +- packages/managed-allocation/src/store.ts | 19 +- .../platform-apple/src/core/runner-host.ts | 3 +- .../__tests__/runner-device-set.test.ts | 133 ++++++ packages/platform-apple/src/runner/host.ts | 11 +- .../src/runner/runner-artifact.ts | 137 +++--- .../platform-apple/src/runner/runner-cache.ts | 25 +- .../src/runner/runner-device-set.ts | 34 +- .../src/runner/runner-disposal.ts | 3 +- .../platform-apple/src/runner/runner-lease.ts | 37 +- .../src/runner/runner-session.ts | 3 +- .../src/agent-browser-tool.test.ts | 35 ++ .../platform-web/src/agent-browser-tool.ts | 74 ++-- .../atomic-publish-ownership.test.ts | 9 +- src/daemon/device-claim-store.ts | 26 +- 20 files changed, 870 insertions(+), 381 deletions(-) create mode 100644 packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts diff --git a/packages/host-kit/src/file.ts b/packages/host-kit/src/file.ts index ce6024283a..e6e359212f 100644 --- a/packages/host-kit/src/file.ts +++ b/packages/host-kit/src/file.ts @@ -12,4 +12,9 @@ export { openVerifiedFileForTruncate, } from './internal/verified-file.ts'; export { expandUserHomePath, resolveUserPath } from './internal/path-resolution.ts'; -export { acquireProcessLock, type ProcessLockOwner } from './internal/process-lock.ts'; +export { + acquireProcessLock, + withProcessLock, + type ProcessLockOwner, + type ProcessLockRelease, +} from './internal/process-lock.ts'; diff --git a/packages/host-kit/src/internal/process-lock.test.ts b/packages/host-kit/src/internal/process-lock.test.ts index 3dc76b7bff..4542225bf4 100644 --- a/packages/host-kit/src/internal/process-lock.test.ts +++ b/packages/host-kit/src/internal/process-lock.test.ts @@ -11,12 +11,15 @@ vi.mock('./host-process.ts', async (importOriginal) => { return { ...actual, isProcessZombie: (pid: number) => zombiePids.has(pid) }; }); -import { acquireProcessLock, type ProcessLockOwner } from './process-lock.ts'; +import { + acquireProcessLock, + withProcessLock, + type ProcessLockOwner, + type ProcessLockRelease, +} from './process-lock.ts'; import { readProcessStartTime } from './host-process.ts'; import { mkdtempForTestSync } from './tmp-dir.fixtures.ts'; -const RECLAIMED_MARK = '.reclaimed-'; - let tmpDir: string; beforeEach(() => { @@ -263,6 +266,9 @@ test('acquireProcessLock reclaims a lock whose owner record was never written', assert.equal(fs.existsSync(lockDirPath), false); }); +// The grace is a second in the future rather than zero: an abandoned mutex is what a zero grace +// would say about the mutex the winner holds, and this test would then be measuring a reclaim +// that skipped it. What the mutex itself is worth is the two mutex tests further down. test('one abandoned lock offered to two contenders is held by exactly one of them', async () => { const lockDirPath = path.join(tmpDir, 'contended.lock'); fs.mkdirSync(lockDirPath); @@ -272,14 +278,14 @@ test('one abandoned lock offered to two contenders is held by exactly one of the acquireProcessLock({ lockDirPath, owner: currentProcessOwner(), - ownerGraceMs: 0, + ownerGraceMs: 1_000, timeoutMs: 250, pollMs: 2, }), acquireProcessLock({ lockDirPath, owner: currentProcessOwner(), - ownerGraceMs: 0, + ownerGraceMs: 1_000, timeoutMs: 250, pollMs: 2, }), @@ -293,13 +299,14 @@ test('one abandoned lock offered to two contenders is held by exactly one of the assert.ok(reason instanceof AppError); assert.equal(reason.details?.ownerLiveness, 'live'); await (acquired[0] as PromiseFulfilledResult<() => Promise>).value(); - assert.deepEqual(listReclaimedSiblings(tmpDir), []); + assert.deepEqual(listReclaimSiblings(tmpDir), []); }); -function listReclaimedSiblings(directory: string): string[] { +/** A reclaim that finishes leaves neither a parked directory nor a mutex behind. */ +function listReclaimSiblings(directory: string): string[] { return fs .readdirSync(directory) - .filter((entry) => entry.includes(RECLAIMED_MARK)) + .filter((entry) => entry.includes('.reclaim')) .sort(); } @@ -374,28 +381,38 @@ test('acquireProcessLock reclaims a stray path in place of the lock directory', assert.equal(fs.existsSync(lockDirPath), false); }); -test('a forced reclaim leaves a directory that a live owner republished', async () => { - const lockDirPath = path.join(tmpDir, 'republished.lock'); +test('a contender that claims the path during a reclaim keeps its lock', async () => { + const lockDirPath = path.join(tmpDir, 'claimed-during-reclaim.lock'); + const mutexPath = path.join(tmpDir, 'claimed-during-reclaim.reclaim.lock'); const ownerFilePath = path.join(lockDirPath, 'owner.json'); fs.mkdirSync(lockDirPath); fs.writeFileSync( ownerFilePath, JSON.stringify({ pid: 999_999_999, startTime: null, acquiredAtMs: Date.now() }), ); + stampDirectoryAbandoned(lockDirPath); - // A win32 handle refuses the rename, and by the time the forced removal would run a - // live holder has claimed the path: the forced removal must not reach its directory. - const realRename = fs.renameSync; - const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation((( - from: fs.PathLike, - to: fs.PathLike, + // The moment a contender is admitted to judging this lock, another process clears the dead + // claim and publishes its own. Nothing is removed: the record re-read under the mutex names a + // claim token the dead one cannot answer to, and the judge walks away from the path. + let claimed = false; + const realMkdir = fs.mkdirSync; + const mkdirSpy = vi.spyOn(fs, 'mkdirSync').mockImplementation((( + target: fs.PathLike, + options?: fs.MakeDirectoryOptions & { recursive: true }, ) => { - if (!String(to).includes(RECLAIMED_MARK)) return realRename(from, to); - fs.rmSync(String(from), { recursive: true, force: true }); - fs.mkdirSync(String(from)); - fs.writeFileSync(path.join(String(from), 'owner.json'), JSON.stringify(currentProcessOwner())); - throw Object.assign(new Error('operation not permitted'), { code: 'EPERM' }); - }) as typeof fs.renameSync); + if (String(target) !== mutexPath || claimed) { + return realMkdir(target as string, options as fs.MakeDirectoryOptions); + } + claimed = true; + fs.rmSync(lockDirPath, { recursive: true, force: true }); + fs.mkdirSync(lockDirPath); + fs.writeFileSync( + ownerFilePath, + JSON.stringify({ ...currentProcessOwner(), claimToken: 'contender-claim' }), + ); + return realMkdir(target as string, options as fs.MakeDirectoryOptions); + }) as typeof fs.mkdirSync); try { await assert.rejects( @@ -409,73 +426,99 @@ test('a forced reclaim leaves a directory that a live owner republished', async (error: unknown) => { assert.ok(error instanceof AppError); assert.equal(error.details?.ownerLiveness, 'live'); + assert.equal(error.details?.ownerPid, process.pid); return true; }, ); - assert.equal(fs.existsSync(ownerFilePath), true); + assert.equal(claimed, true); + const record = JSON.parse(fs.readFileSync(ownerFilePath, 'utf8')) as { + pid: number; + claimToken: string; + }; + assert.equal(record.pid, process.pid); + assert.equal(record.claimToken, 'contender-claim'); } finally { - renameSpy.mockRestore(); + mkdirSpy.mockRestore(); } }); -test('a reclaim that cannot remove the directory it moved aside still holds the lock', async () => { - const lockDirPath = path.join(tmpDir, 'immovable.lock'); +// An abandoned directory with no record is the one thing this module deletes on age alone, so the +// two facts it re-checks under the mutex need their own witnesses: what is inside now, and how old +// the directory now is. +test('a claim published while a reclaim holds the mutex outlives the empty directory it filled', async () => { + const lockDirPath = path.join(tmpDir, 'filled-during-reclaim.lock'); + const mutexPath = path.join(tmpDir, 'filled-during-reclaim.reclaim.lock'); + const ownerFilePath = path.join(lockDirPath, 'owner.json'); fs.mkdirSync(lockDirPath); - fs.writeFileSync( - path.join(lockDirPath, 'owner.json'), - JSON.stringify({ pid: 999_999_999, startTime: null, acquiredAtMs: Date.now() }), - ); stampDirectoryAbandoned(lockDirPath); - const realRemove = fs.rmSync; - const removeSpy = vi.spyOn(fs, 'rmSync').mockImplementation(((target: fs.PathLike, options) => { - if (String(target).includes(RECLAIMED_MARK)) { - throw Object.assign(new Error('directory is busy'), { code: 'EBUSY' }); + // Writing the record is also what re-dates the directory, which is the fact the reclaim re-asks + // for under its mutex before it removes anything. + let published = false; + const realMkdir = fs.mkdirSync; + const mkdirSpy = vi.spyOn(fs, 'mkdirSync').mockImplementation((( + target: fs.PathLike, + options?: fs.MakeDirectoryOptions & { recursive: true }, + ) => { + if (String(target) !== mutexPath || published) { + return realMkdir(target as string, options as fs.MakeDirectoryOptions); } - return realRemove(target, options); - }) as typeof fs.rmSync); + published = true; + fs.writeFileSync( + ownerFilePath, + JSON.stringify({ ...currentProcessOwner(), claimToken: 'late-claim' }), + ); + return realMkdir(target as string, options as fs.MakeDirectoryOptions); + }) as typeof fs.mkdirSync); try { - const release = await acquireProcessLock({ - lockDirPath, - owner: currentProcessOwner(), - timeoutMs: 500, - pollMs: 1, - }); - assert.equal(fs.existsSync(path.join(lockDirPath, 'owner.json')), true); - await release(); + await assert.rejects( + () => + acquireProcessLock({ + lockDirPath, + owner: { pid: 999_999_998, startTime: null, acquiredAtMs: Date.now() }, + timeoutMs: 50, + pollMs: 1, + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.ownerLiveness, 'live'); + return true; + }, + ); + assert.equal(published, true); + assert.equal(fs.existsSync(lockDirPath), true); + const record = JSON.parse(fs.readFileSync(ownerFilePath, 'utf8')) as { claimToken: string }; + assert.equal(record.claimToken, 'late-claim'); } finally { - removeSpy.mockRestore(); + mkdirSpy.mockRestore(); } }); -test('a live owner published between the stale read and the rename keeps its lock', async () => { - const lockDirPath = path.join(tmpDir, 'stolen-race.lock'); - const ownerFilePath = path.join(lockDirPath, 'owner.json'); +test('a lock directory made anew while a reclaim holds the mutex is not the one that was abandoned', async () => { + const lockDirPath = path.join(tmpDir, 'refilled-during-reclaim.lock'); + const mutexPath = path.join(tmpDir, 'refilled-during-reclaim.reclaim.lock'); fs.mkdirSync(lockDirPath); - fs.writeFileSync( - ownerFilePath, - JSON.stringify({ pid: 999_999_999, startTime: null, acquiredAtMs: Date.now() }), - ); stampDirectoryAbandoned(lockDirPath); - // The dead record is read, and before the rename lands another contender reclaims the - // path, publishes itself, and goes live. The rename then moves that live directory, and - // the only thing that can tell it apart from the one judged abandoned is the directory - // itself. - let republished = false; - const realRename = fs.renameSync; - const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation((( - from: fs.PathLike, - to: fs.PathLike, + // Replacing the directory rather than filling it is what a contender that won the path looks + // like from the inside: same name, same emptiness, and an age that says it was never abandoned. + let replaced = false; + let refilledAtMs = 0; + const realMkdir = fs.mkdirSync; + const mkdirSpy = vi.spyOn(fs, 'mkdirSync').mockImplementation((( + target: fs.PathLike, + options?: fs.MakeDirectoryOptions & { recursive: true }, ) => { - if (!String(to).includes(RECLAIMED_MARK) || republished) return realRename(from, to); - republished = true; - fs.rmSync(String(from), { recursive: true, force: true }); - fs.mkdirSync(String(from)); - fs.writeFileSync(path.join(String(from), 'owner.json'), JSON.stringify(currentProcessOwner())); - return realRename(from, to); - }) as typeof fs.renameSync); + if (String(target) !== mutexPath || replaced) { + return realMkdir(target as string, options as fs.MakeDirectoryOptions); + } + replaced = true; + fs.rmSync(lockDirPath, { recursive: true, force: true }); + fs.mkdirSync(lockDirPath); + refilledAtMs = fs.statSync(lockDirPath).mtimeMs; + return realMkdir(target as string, options as fs.MakeDirectoryOptions); + }) as typeof fs.mkdirSync); try { await assert.rejects( @@ -488,66 +531,217 @@ test('a live owner published between the stale read and the rename keeps its loc }), (error: unknown) => { assert.ok(error instanceof AppError); - assert.equal(error.details?.ownerLiveness, 'live'); - assert.equal(error.details?.ownerPid, process.pid); return true; }, ); - assert.equal(republished, true); + assert.equal(replaced, true); assert.equal( - (JSON.parse(fs.readFileSync(ownerFilePath, 'utf8')) as { pid: number }).pid, - process.pid, + fs.statSync(lockDirPath).mtimeMs, + refilledAtMs, + 'the reclaim removed a directory it had not judged abandoned', ); - assert.deepEqual( - fs - .readdirSync(tmpDir) - .filter((name) => name.includes(RECLAIMED_MARK)) - .sort(), - [], + } finally { + mkdirSpy.mockRestore(); + } +}); + +test('a reclaim mutex another contender holds leaves the abandoned lock standing', async () => { + const lockDirPath = path.join(tmpDir, 'judged-by-another.lock'); + const ownerFilePath = path.join(lockDirPath, 'owner.json'); + fs.mkdirSync(lockDirPath); + const staleClaim = { pid: 999_999_999, startTime: null, acquiredAtMs: Date.now() }; + fs.writeFileSync(ownerFilePath, JSON.stringify(staleClaim)); + stampDirectoryAbandoned(lockDirPath); + fs.mkdirSync(path.join(tmpDir, 'judged-by-another.reclaim.lock')); + + // Nobody may judge this lock twice, and a contender that cannot say so keeps polling + // rather than clearing what it has not finished reading. The grace is the default five seconds, + // so the mutex this test placed is held rather than abandoned. + let lockAttempts = 0; + const realMkdir = fs.mkdirSync; + const mkdirSpy = vi.spyOn(fs, 'mkdirSync').mockImplementation((( + target: fs.PathLike, + options?: fs.MakeDirectoryOptions & { recursive: true }, + ) => { + if (String(target) === lockDirPath) lockAttempts += 1; + return realMkdir(target as string, options as fs.MakeDirectoryOptions); + }) as typeof fs.mkdirSync); + + try { + await assert.rejects( + () => + acquireProcessLock({ + lockDirPath, + owner: { pid: 999_999_998, startTime: null, acquiredAtMs: Date.now() }, + timeoutMs: 50, + pollMs: 1, + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.ownerPid, 999_999_999); + return true; + }, ); } finally { - renameSpy.mockRestore(); + mkdirSpy.mockRestore(); } + assert.ok(lockAttempts > 1, `contender polled ${lockAttempts} times`); + assert.equal(fs.existsSync(ownerFilePath), true); }); -test('a reclaim whose directory another contender moved aside retries and acquires', async () => { - const lockDirPath = path.join(tmpDir, 'lost-race.lock'); +test('a reclaim mutex left behind by a dead process is cleared by age', async () => { + const lockDirPath = path.join(tmpDir, 'dead-janitor.lock'); + const mutexPath = path.join(tmpDir, 'dead-janitor.reclaim.lock'); fs.mkdirSync(lockDirPath); fs.writeFileSync( path.join(lockDirPath, 'owner.json'), JSON.stringify({ pid: 999_999_999, startTime: null, acquiredAtMs: Date.now() }), ); + fs.mkdirSync(mutexPath); stampDirectoryAbandoned(lockDirPath); + stampDirectoryAbandoned(mutexPath); - // The contender that loses the rename finds the path already gone and cannot have - // cleared anything; it goes back to `mkdir`, which is what decides the lock. - let attempted = 0; - const realRename = fs.renameSync; - const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation((( - from: fs.PathLike, - to: fs.PathLike, - ) => { - if (!String(to).includes(RECLAIMED_MARK)) return realRename(from, to); - attempted += 1; - fs.rmSync(lockDirPath, { recursive: true, force: true }); - throw Object.assign(new Error('no such directory'), { code: 'ENOENT' }); - }) as typeof fs.renameSync); + const release = await acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + timeoutMs: 500, + pollMs: 1, + }); + + assert.equal(fs.existsSync(path.join(lockDirPath, 'owner.json')), true); + assert.equal(fs.existsSync(mutexPath), false); + await release(); +}); + +test('a reclaim that cannot clear the lock directory leaves the record it judged', async () => { + const lockDirPath = path.join(tmpDir, 'immovable.lock'); + const ownerFilePath = path.join(lockDirPath, 'owner.json'); + fs.mkdirSync(lockDirPath); + fs.writeFileSync( + ownerFilePath, + JSON.stringify({ pid: 999_999_999, startTime: null, acquiredAtMs: Date.now() }), + ); + stampDirectoryAbandoned(lockDirPath); + + const realRemove = fs.rmSync; + const removeSpy = vi.spyOn(fs, 'rmSync').mockImplementation(((target: fs.PathLike) => { + if (String(target) !== lockDirPath) { + return realRemove(target as string); + } + throw Object.assign(new Error('directory is busy'), { code: 'EBUSY' }); + }) as typeof fs.rmSync); try { - const release = await acquireProcessLock({ - lockDirPath, - owner: currentProcessOwner(), - timeoutMs: 500, - pollMs: 1, - }); - assert.equal(attempted, 1); - assert.equal(fs.existsSync(path.join(lockDirPath, 'owner.json')), true); - await release(); + await assert.rejects( + () => + acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + timeoutMs: 50, + pollMs: 1, + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.ownerPid, 999_999_999); + return true; + }, + ); + assert.equal(fs.existsSync(ownerFilePath), true); } finally { - renameSpy.mockRestore(); + removeSpy.mockRestore(); } }); +test('an abandoned lock with no record and something else inside is left alone', async () => { + const lockDirPath = path.join(tmpDir, 'occupied.lock'); + const strangerPath = path.join(lockDirPath, 'not-a-record.json'); + fs.mkdirSync(lockDirPath); + fs.writeFileSync(strangerPath, 'nobody claims this'); + stampDirectoryAbandoned(lockDirPath); + // Stamping the directory's own clocks back makes the stranger look older than the grace, too. + fs.utimesSync(strangerPath, new Date(Date.now() - 60_000), new Date(Date.now() - 60_000)); + + // No record means no claim to attribute the directory to, and the age of the path is + // evidence about the path alone. An empty directory is removed; this one is not. + await assert.rejects( + () => + acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + timeoutMs: 50, + pollMs: 1, + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + return true; + }, + ); + assert.equal(fs.existsSync(strangerPath), true); +}); + +test('withProcessLock gives the lock back on every path out of the task', async () => { + const releases: string[] = []; + const release: ProcessLockRelease = async () => { + releases.push('released'); + }; + + await withProcessLock({ + acquire: async () => release, + task: async () => 'done', + }); + await assert.rejects( + () => + withProcessLock({ + acquire: async () => release, + task: async () => { + throw new Error('task failed'); + }, + }), + /task failed/, + ); + + assert.deepEqual(releases, ['released', 'released']); +}); + +test('a task that failed is reported over a release that could not verify ownership', async () => { + await assert.rejects( + () => + withProcessLock({ + acquire: async () => async () => { + throw new AppError('COMMAND_FAILED', 'Cannot verify ownership of device claim', { + ownerReleaseUnverified: true, + }); + }, + task: async () => { + throw new Error('the write was rejected'); + }, + }), + (error: unknown) => { + assert.equal((error as Error).message, 'the write was rejected'); + return true; + }, + ); +}); + +test('a completed task still reports a lock it could not give back', async () => { + await assert.rejects( + () => + withProcessLock({ + acquire: async () => async () => { + throw new AppError('COMMAND_FAILED', 'Cannot verify ownership of device claim', { + ownerReleaseUnverified: true, + }); + }, + task: async () => 'done', + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.ownerReleaseUnverified, true); + return true; + }, + ); +}); + function stampDirectoryAbandoned(directory: string): void { const abandoned = new Date(Date.now() - 60_000); fs.utimesSync(directory, abandoned, abandoned); diff --git a/packages/host-kit/src/internal/process-lock.ts b/packages/host-kit/src/internal/process-lock.ts index 247a6a2c6f..942dd87dc9 100644 --- a/packages/host-kit/src/internal/process-lock.ts +++ b/packages/host-kit/src/internal/process-lock.ts @@ -11,6 +11,7 @@ const DEFAULT_LOCK_TIMEOUT_MS = 30_000; const DEFAULT_LOCK_POLL_MS = 100; const DEFAULT_LOCK_OWNER_GRACE_MS = 5_000; const LOCK_DIRECTORY_SUFFIX = '.lock'; +const RECLAIM_MUTEX_SUFFIX = '.reclaim'; export type ProcessLockOwner = { pid: number; @@ -27,6 +28,33 @@ export type ProcessLockOwnerRecord = ProcessLockOwner & { claimToken: string | null; }; +/** Gives a lock back. Rejects when the lock is standing and this process cannot prove it owns it. */ +export type ProcessLockRelease = () => Promise; + +/** + * Runs `task` while the lock that `acquire` returns is held, and settles the question every + * caller otherwise answers by hand: which of two failures to report. + * + * A task that failed is the reportable fact, and an unverified release afterwards only says the + * lock is still standing, which the stale-clear path resolves on its own. On the success path + * the release is not best effort: a lock this process could not give back is not a completed + * task, and swallowing it would report success while the next contender waits. + */ +export async function withProcessLock(params: { + acquire: () => Promise; + task: () => Promise; +}): Promise { + const release = await params.acquire(); + try { + const result = await params.task(); + await release(); + return result; + } catch (error) { + await release().catch(() => undefined); + throw error; + } +} + type ProcessLockOwnerReading = | { kind: 'owner'; owner: ProcessLockOwnerRecord } | { kind: 'unwritten' } @@ -39,7 +67,7 @@ export async function acquireProcessLock(params: { pollMs?: number; ownerGraceMs?: number; description?: string; -}): Promise<() => Promise> { +}): Promise { const { lockDirPath, owner } = params; const ownerFilePath = path.join(lockDirPath, OWNER_FILE_NAME); const deadline = Date.now() + (params.timeoutMs ?? DEFAULT_LOCK_TIMEOUT_MS); @@ -115,11 +143,30 @@ function releaseProcessLock( if (reading.kind === 'unreadable') return 'unverified'; if (reading.kind === 'unwritten' || !ownerIdentityMatches(reading.owner, claim)) return 'not-owner'; - // The same process can hold this path twice in sequence, and a reclaim that moved our - // directory aside leaves a record behind that names us as though nothing had happened. + // The same process can hold this path twice in sequence, so the token is what tells this + // acquisition's record from an earlier one that names the very same process. if (reading.owner.claimToken !== claim.claimToken) return 'not-owner'; - fs.rmSync(lockDirPath, { recursive: true, force: true }); - return 'removed'; + return clearLockDirectory(lockDirPath, ownerFilePath); +} + +/** + * A lock directory is written by this module and holds one file: its record. So it is emptied + * and removed rather than removed with everything inside, and a directory that turns out to + * hold something else is left standing. That distinction is the difference between clearing a + * lock and destroying whoever put their thing in that path. + */ +function clearLockDirectory(lockDirPath: string, ownerFilePath: string): 'removed' | 'unverified' { + try { + fs.unlinkSync(ownerFilePath); + } catch (error) { + if (errorCode(error) !== 'ENOENT') return 'unverified'; + } + try { + fs.rmdirSync(lockDirPath); + return 'removed'; + } catch (error) { + return errorCode(error) === 'ENOENT' ? 'removed' : 'unverified'; + } } function clearStaleProcessLock( @@ -137,23 +184,23 @@ function clearStaleProcessLock( // A lock path held by anything that is not a directory cannot carry a readable // owner record, so its age is the only evidence available about it. if (!lockStats.isDirectory()) { - return reclaimWhenAbandoned(lockStats, ownerGraceMs) - ? reclaimProcessLockDirectory(lockDirPath, ownerFilePath, { - stats: lockStats, - claimToken: null, - }) - : false; + return ( + reclaimWhenAbandoned(lockStats, ownerGraceMs) && + reclaimLockUnderMutex(lockDirPath, ownerFilePath, ownerGraceMs, { kind: 'stray' }) + ); } const reading = readProcessLockOwner(ownerFilePath); if (reading.kind === 'owner') { - if (isLiveProcessLockOwner(reading.owner)) { - return false; - } - return reclaimProcessLockDirectory(lockDirPath, ownerFilePath, { - stats: lockStats, - claimToken: reading.owner.claimToken, - }); + // A record identifies the acquisition that wrote it, so the directory around a claim judged + // dead is that claim's property. + return ( + !isLiveProcessLockOwner(reading.owner) && + reclaimLockUnderMutex(lockDirPath, ownerFilePath, ownerGraceMs, { + kind: 'dead-claim', + claimToken: reading.owner.claimToken, + }) + ); } // A record we cannot read leaves an owner whose identity is unknown, which is not // evidence of death. Only a record that is genuinely absent lets the directory's @@ -161,12 +208,10 @@ function clearStaleProcessLock( if (reading.kind === 'unreadable') { return false; } - return reclaimWhenAbandoned(lockStats, ownerGraceMs) - ? reclaimProcessLockDirectory(lockDirPath, ownerFilePath, { - stats: lockStats, - claimToken: null, - }) - : false; + return ( + reclaimWhenAbandoned(lockStats, ownerGraceMs) && + reclaimLockUnderMutex(lockDirPath, ownerFilePath, ownerGraceMs, { kind: 'empty' }) + ); } function reclaimWhenAbandoned(lockStats: fs.Stats, ownerGraceMs: number): boolean { @@ -174,100 +219,151 @@ function reclaimWhenAbandoned(lockStats: fs.Stats, ownerGraceMs: number): boolea } /** - * Moves the abandoned directory aside under a unique name before removing it, so - * exactly one contender can reclaim one lock. Two plain removals look different from - * the caller's side: the second succeeds silently on the path the first already - * cleared, and both contenders continue as though they had freed the lock. `mkdir`'s - * `EEXIST`, unchanged, stays the arbiter of the lock itself. + * What the judgement outside the mutex found, in the one form the removal decision needs: a claim + * whose owner is dead, a directory that has never held a record, or a path that is not one. + */ +type JudgedLock = + | { kind: 'dead-claim'; claimToken: ProcessLockOwnerRecord['claimToken'] } + | { kind: 'empty' } + | { kind: 'stray' }; + +/** + * An abandoned lock is the one directory this module destroys without having created it, and + * two contenders that both remove it independently both walk away believing they freed the + * path. So the decision is taken again inside a mutex of its own, aged by the same grace as the + * lock it guards, and everyone who cannot hold it keeps polling. + * + * Nothing is moved out of the way first: an absent lock path is an invitation, and a lock parked + * under another name would return to a path somebody else already wrote a record on. Each branch + * re-decides from what is on disk now and removes in place, and a path that has already gone is + * left untouched so the caller's next `mkdir` simply wins it. */ -function reclaimProcessLockDirectory( +function reclaimLockUnderMutex( lockDirPath: string, ownerFilePath: string, + ownerGraceMs: number, judged: JudgedLock, ): boolean { - const asidePath = reclaimedLockPath(lockDirPath); + if (!holdReclaimMutex(lockDirPath, ownerGraceMs)) return false; try { - fs.renameSync(lockDirPath, asidePath); - } catch (error) { - return reclaimWithoutTheRename(lockDirPath, ownerFilePath, error); - } - if (!reclaimedLockIsTheOneJudged(asidePath, judged)) { - restoreReclaimedLockDirectory(asidePath, lockDirPath); - return false; + switch (judged.kind) { + case 'dead-claim': + return removeDeadClaimLock(lockDirPath, ownerFilePath, judged.claimToken); + case 'empty': + return removeAbandonedEmptyLock(lockDirPath, ownerGraceMs); + case 'stray': + return removeStrayLockPath(lockDirPath); + } + } finally { + releaseReclaimMutex(lockDirPath); } - removeReclaimedLockDirectory(asidePath); - return true; } /** - * A win32 directory with a handle open inside it refuses the rename and often the removal - * too, which is why a forced removal remains as the fallback: stale-clear atomicity is - * best effort there and exact elsewhere. The fallback re-reads the record first, because a - * refused rename means time passed and a live contender may have claimed the path in it. + * The mutex says no other contender is reclaiming. It says nothing about the lock's owner, who + * may have released the path and handed it to someone new while this process took the mutex, so + * the record decides: a claim token is a random id no later acquisition can repeat, and a + * directory whose record carries any other token, or no record at all, belongs to somebody else. */ -function reclaimWithoutTheRename( +function removeDeadClaimLock( lockDirPath: string, ownerFilePath: string, - renameError: unknown, + claimToken: ProcessLockOwnerRecord['claimToken'], ): boolean { - const code = (renameError as NodeJS.ErrnoException | null)?.code; - if (code === 'ENOENT') return true; - if (code !== 'EPERM' && code !== 'EACCES' && code !== 'ENOTEMPTY') return false; const reading = readProcessLockOwner(ownerFilePath); - if (reading.kind === 'owner' && isLiveProcessLockOwner(reading.owner)) return false; - fs.rmSync(lockDirPath, { recursive: true, force: true }); - return true; + if (reading.kind !== 'owner' || reading.owner.claimToken !== claimToken) return false; + try { + fs.rmSync(lockDirPath, { recursive: true, force: true }); + return true; + } catch { + return false; + } +} + +/** + * A directory that has never held a record has no claim to attribute its contents to, so `rmdir` + * is the only call made on it: it cannot destroy what a new owner published between the judgement + * and here, and `ENOTEMPTY` is that publication saying so. Age re-speaks for the same reason — a + * directory created a moment ago is an acquisition that has not published yet, not an abandoned one. + */ +function removeAbandonedEmptyLock(lockDirPath: string, ownerGraceMs: number): boolean { + let current: fs.Stats; + try { + current = fs.statSync(lockDirPath); + } catch { + return false; + } + if (!current.isDirectory() || !reclaimWhenAbandoned(current, ownerGraceMs)) return false; + try { + fs.rmdirSync(lockDirPath); + return true; + } catch { + return false; + } } /** - * A rename addresses whatever stands at the path now, not the directory whose record was - * read. A contender that reclaimed first and claimed the path again has put a different - * directory there, so what arrived is compared against what was judged: the same inode, - * no live owner inside, and the claim token that was read before the rename. + * `unlink` answers `EISDIR` for a directory, which is a claim this process never held, so the + * system call itself refuses the one case this branch must not touch. */ -function reclaimedLockIsTheOneJudged(asidePath: string, judged: JudgedLock): boolean { - let moved: fs.Stats; +function removeStrayLockPath(lockDirPath: string): boolean { try { - moved = fs.statSync(asidePath); + fs.unlinkSync(lockDirPath); + return true; } catch { return false; } - if (moved.ino !== judged.stats.ino || moved.dev !== judged.stats.dev) return false; - const reading = readProcessLockOwner(path.join(asidePath, OWNER_FILE_NAME)); - if (reading.kind === 'owner' && isLiveProcessLockOwner(reading.owner)) return false; - return readClaimToken(reading) === judged.claimToken; } -type JudgedLock = { - stats: fs.Stats; - claimToken: string | null; -}; +/** Keeps the `.lock` suffix so a sibling scanner still reads the name as a lock. */ +function reclaimMutexPath(lockDirPath: string): string { + const stem = lockDirPath.endsWith(LOCK_DIRECTORY_SUFFIX) + ? lockDirPath.slice(0, -LOCK_DIRECTORY_SUFFIX.length) + : lockDirPath; + return `${stem}${RECLAIM_MUTEX_SUFFIX}${LOCK_DIRECTORY_SUFFIX}`; +} + +function holdReclaimMutex(lockDirPath: string, abandonedAfterMs: number): boolean { + const mutexPath = reclaimMutexPath(lockDirPath); + for (let attempt = 0; attempt < 2; attempt += 1) { + try { + fs.mkdirSync(mutexPath); + return true; + } catch (error) { + if (errorCode(error) !== 'EEXIST') throw error; + if (!clearAbandonedReclaimMutex(mutexPath, abandonedAfterMs)) return false; + } + } + return false; +} /** - * Returns a directory that turned out to belong to someone else. Failure is not a - * licence to delete it: its holder's own release reports an unreadable owner rather than - * let this process clear a lock it does not own. + * A mutex left behind by a process that died mid-reclaim is cleared by age, the same evidence + * an abandoned lock is judged by. Two contenders may both decide to clear it; the `mkdir` that + * follows still admits one of them. */ -function restoreReclaimedLockDirectory(asidePath: string, lockDirPath: string): void { +function clearAbandonedReclaimMutex(mutexPath: string, abandonedAfterMs: number): boolean { + let stats: fs.Stats; + try { + stats = fs.statSync(mutexPath); + } catch { + return true; + } + if (Date.now() - stats.mtimeMs < abandonedAfterMs) return false; try { - fs.renameSync(asidePath, lockDirPath); + fs.rmdirSync(mutexPath); } catch {} + return true; } -/** Janitorial work after the rename already achieved the exclusion. */ -function removeReclaimedLockDirectory(asidePath: string): void { +function releaseReclaimMutex(lockDirPath: string): void { try { - fs.rmSync(asidePath, { recursive: true, force: true }); + fs.rmdirSync(reclaimMutexPath(lockDirPath)); } catch {} } -/** Keeps the `.lock` suffix so a sibling scanner still reads the name as a lock. */ -function reclaimedLockPath(lockDirPath: string): string { - const token = `${process.pid}-${crypto.randomUUID()}`; - const stem = lockDirPath.endsWith(LOCK_DIRECTORY_SUFFIX) - ? lockDirPath.slice(0, -LOCK_DIRECTORY_SUFFIX.length) - : lockDirPath; - return `${stem}.reclaimed-${token}${LOCK_DIRECTORY_SUFFIX}`; +function errorCode(error: unknown): string | undefined { + return (error as NodeJS.ErrnoException | null)?.code; } /** @@ -287,10 +383,6 @@ function readProcessLockOwner(ownerFilePath: string): ProcessLockOwnerReading { return owner ? { kind: 'owner', owner } : { kind: 'unreadable' }; } -function readClaimToken(reading: ProcessLockOwnerReading): string | null { - return reading.kind === 'owner' ? reading.owner.claimToken : null; -} - function parseProcessLockOwner(contents: string): ProcessLockOwnerRecord | null { let parsed: unknown; try { diff --git a/packages/managed-allocation/src/__tests__/store.test.ts b/packages/managed-allocation/src/__tests__/store.test.ts index e02f11f59c..c0c1e1b0da 100644 --- a/packages/managed-allocation/src/__tests__/store.test.ts +++ b/packages/managed-allocation/src/__tests__/store.test.ts @@ -126,13 +126,13 @@ test('retains corrupt, unsupported, unfenced, and path-ambiguous records as diag assert.equal(path.dirname(path.dirname(operationPath)), path.join(root, 'allocations')); }); -test('listing a lane lock and the directory a stale reclaim renamed aside finds no extra record', () => { +test('listing a lane lock and the mutex a stale reclaim holds finds no extra record', () => { const { root, store, record } = fixture(); store.create(record); const listed = store.list(); const allocationsDir = path.join(root, 'allocations'); const lane = path.basename(path.dirname(store.resolvePath(record))); - for (const lockName of [`${lane}.lane.lock`, `${lane}.reclaimed-4242-0.lock`]) { + for (const lockName of [`${lane}.lane.lock`, `${lane}.lane.reclaim.lock`]) { const lockDir = path.join(allocationsDir, lockName); fs.mkdirSync(lockDir, { recursive: true }); fs.writeFileSync(path.join(lockDir, 'owner.json'), JSON.stringify({ pid: process.pid })); diff --git a/packages/managed-allocation/src/store-filesystem.ts b/packages/managed-allocation/src/store-filesystem.ts index 425702d2cf..e08ef91955 100644 --- a/packages/managed-allocation/src/store-filesystem.ts +++ b/packages/managed-allocation/src/store-filesystem.ts @@ -114,8 +114,8 @@ function readDirectory(directory: string): DirectoryRead { } function listLanePaths(allocationsDir: string, lane: fs.Dirent): AllocationOperationPath[] { - // Lane and operation locks sit beside the records, and a stale reclaim renames one - // aside under its own name; none of those directories holds an operation record. + // Lane and operation locks sit beside the records, including the `.reclaim` mutex a stale + // reclaim holds while it decides; none of those directories holds an operation record. if (lane.name.endsWith('.lock')) return []; const lanePath = path.join(allocationsDir, lane.name); if (!lane.isDirectory()) { diff --git a/packages/managed-allocation/src/store-lock.ts b/packages/managed-allocation/src/store-lock.ts index 8c0ce0bda3..c33d221444 100644 --- a/packages/managed-allocation/src/store-lock.ts +++ b/packages/managed-allocation/src/store-lock.ts @@ -1,6 +1,6 @@ import crypto from 'node:crypto'; import path from 'node:path'; -import { acquireProcessLock } from '@agent-device/host-kit/file'; +import { acquireProcessLock, withProcessLock } from '@agent-device/host-kit/file'; import { readCurrentOwnerIdentity } from '@agent-device/host-kit/process'; import type { AllocationOperationStore } from './store.ts'; @@ -18,15 +18,14 @@ async function withAllocationLaneLock( requesterId: string, task: () => Promise, ): Promise { - const release = await acquireAllocationStoreLock( - path.join(allocationsDir, `${hash(requesterId)}.lane.lock`), - `allocation lane ${requesterId}`, - ); - try { - return await task(); - } finally { - await release(); - } + return await withProcessLock({ + acquire: () => + acquireAllocationStoreLock( + path.join(allocationsDir, `${hash(requesterId)}.lane.lock`), + `allocation lane ${requesterId}`, + ), + task, + }); } export function acquireAllocationStoreLock( diff --git a/packages/managed-allocation/src/store.ts b/packages/managed-allocation/src/store.ts index ab3abd0eb6..30e85afe8d 100644 --- a/packages/managed-allocation/src/store.ts +++ b/packages/managed-allocation/src/store.ts @@ -1,6 +1,6 @@ import fs from 'node:fs'; import path from 'node:path'; -import { openVerifiedFileForRead } from '@agent-device/host-kit/file'; +import { openVerifiedFileForRead, withProcessLock } from '@agent-device/host-kit/file'; import type { AllocationOperationRecord, AllocationOperationRef, @@ -85,15 +85,14 @@ async function transitionRecord( nowMs: number, ): Promise { const recordPath = operationPath(allocationsDir, ref); - const release = await acquireAllocationStoreLock( - `${recordPath}.lock`, - `allocation operation ${ref.requesterId}/${ref.attemptKey}`, - ); - try { - return applyStoredTransition(recordPath, ref, expectedFence, transition, nowMs); - } finally { - await release(); - } + return await withProcessLock({ + acquire: () => + acquireAllocationStoreLock( + `${recordPath}.lock`, + `allocation operation ${ref.requesterId}/${ref.attemptKey}`, + ), + task: async () => applyStoredTransition(recordPath, ref, expectedFence, transition, nowMs), + }); } function applyStoredTransition( diff --git a/packages/platform-apple/src/core/runner-host.ts b/packages/platform-apple/src/core/runner-host.ts index 12b73caa33..2be8d08fb4 100644 --- a/packages/platform-apple/src/core/runner-host.ts +++ b/packages/platform-apple/src/core/runner-host.ts @@ -1,5 +1,5 @@ import type { AppleRunnerHost } from '../runner/index.ts'; -import { publishFileSync, acquireProcessLock } from '@agent-device/host-kit/file'; +import { publishFileSync, acquireProcessLock, withProcessLock } from '@agent-device/host-kit/file'; import { resolveIosSimulatorDeviceSetPath } from '@agent-device/kernel/device-isolation'; import { @@ -73,6 +73,7 @@ export const appleRunnerHost: AppleRunnerHost = { findProjectRoot, readVersion, acquireProcessLock, + withProcessLock, withKeyedLock, publishFileSync, classifyOwnerLiveness, diff --git a/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts new file mode 100644 index 0000000000..f7226c49da --- /dev/null +++ b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts @@ -0,0 +1,133 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { test } 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'; + +// A runner build runs under the XCTest device-set redirect, which is a lock like any other, so +// the two failures it can report have an order: the build that failed outranks a redirect it could +// not hand back, and a build that succeeded does not get to hide one. + +const iosSimulator: DeviceInfo = { + platform: 'apple', + id: 'sim-1', + name: 'iPhone Simulator', + kind: 'simulator', + appleOs: 'ios', + booted: true, +}; + +type RedirectPaths = { + requestedSetPath: string; + xctestDeviceSetPath: string; + lockDirPath: string; +}; + +function makeRedirectPaths(root: string): RedirectPaths { + return { + requestedSetPath: path.join(root, 'requested'), + xctestDeviceSetPath: path.join(root, 'Library', 'Developer', 'XCTestDevices'), + lockDirPath: path.join(root, '.agent-device', 'xctest-device-set.lock'), + }; +} + +function redirectOptions(paths: RedirectPaths) { + return { + lockDirPath: paths.lockDirPath, + xctestDeviceSetPath: paths.xctestDeviceSetPath, + }; +} + +function makeScopedSimulator(paths: RedirectPaths): DeviceInfo { + return { ...iosSimulator, simulatorSetPath: paths.requestedSetPath }; +} + +/** The lock is standing and its record cannot be read, which is what no release can forgive. */ +function makeReleaseUnverifiable(paths: RedirectPaths): void { + const ownerFilePath = path.join(paths.lockDirPath, 'owner.json'); + fs.rmSync(ownerFilePath); + fs.mkdirSync(ownerFilePath); +} + +async function withTempDir(prefix: string, task: (root: string) => Promise): Promise { + const root = mkdtempForTestSync(prefix); + try { + return await task(root); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } +} + +test('a build that failed outranks the redirect it could not give back', async () => { + await withTempDir('device-set-build-error-', async (root) => { + const paths = makeRedirectPaths(root); + fs.mkdirSync(paths.requestedSetPath, { recursive: true }); + const buildFailure = new AppError('COMMAND_FAILED', 'xcodebuild build-for-testing failed', { + hint: 'See the runner log.', + }); + + await assert.rejects( + () => + withXcodebuildSimulatorSetRedirect( + makeScopedSimulator(paths), + async () => { + makeReleaseUnverifiable(paths); + throw buildFailure; + }, + redirectOptions(paths), + ), + (error: unknown) => { + assert.equal(error, buildFailure); + return true; + }, + ); + assert.equal(fs.existsSync(paths.lockDirPath), true); + }); +}); + +test('a build that succeeded still reports the redirect it could not give back', async () => { + await withTempDir('device-set-release-error-', async (root) => { + const paths = makeRedirectPaths(root); + fs.mkdirSync(paths.requestedSetPath, { recursive: true }); + + await assert.rejects( + () => + withXcodebuildSimulatorSetRedirect( + makeScopedSimulator(paths), + async () => { + makeReleaseUnverifiable(paths); + return 'built'; + }, + redirectOptions(paths), + ), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.ownerReleaseUnverified, true); + return true; + }, + ); + }); +}); + +test('a redirect handed back after its task keeps quiet about a release it cannot verify', async () => { + await withTempDir('device-set-teardown-', async (root) => { + const paths = makeRedirectPaths(root); + fs.mkdirSync(paths.requestedSetPath, { recursive: true }); + const redirect = await acquireXcodebuildSimulatorSetRedirect(makeScopedSimulator(paths), { + lockDirPath: paths.lockDirPath, + xctestDeviceSetPath: paths.xctestDeviceSetPath, + }); + assert.notEqual(redirect, null); + makeReleaseUnverifiable(paths); + + await releaseXcodebuildSimulatorSetRedirectBestEffort(redirect); + assert.equal(fs.existsSync(paths.lockDirPath), true); + }); +}); diff --git a/packages/platform-apple/src/runner/host.ts b/packages/platform-apple/src/runner/host.ts index 20eea8b285..3054afa9df 100644 --- a/packages/platform-apple/src/runner/host.ts +++ b/packages/platform-apple/src/runner/host.ts @@ -121,6 +121,9 @@ export type ProcessLockOwner = { acquiredAtMs: number; }; +/** Hands a lock back. Rejects when the lock is standing and this process cannot prove it owns it. */ +export type ProcessLockRelease = () => Promise; + export type OwnerLiveness = | 'live' | 'owner-process-dead' @@ -203,7 +206,11 @@ export type AppleRunnerHost = { pollMs?: number; ownerGraceMs?: number; description?: string; - }): Promise<() => Promise>; + }): Promise; + withProcessLock(params: { + acquire: () => Promise; + task: () => Promise; + }): Promise; withKeyedLock( locks: Map>, key: string, @@ -335,6 +342,8 @@ export const readVersion: AppleRunnerHost['readVersion'] = (root) => requireHost().readVersion(root); export const acquireProcessLock: AppleRunnerHost['acquireProcessLock'] = (params) => requireHost().acquireProcessLock(params); +export const withProcessLock: AppleRunnerHost['withProcessLock'] = (params) => + requireHost().withProcessLock(params); export const withKeyedLock = ( locks: Map>, key: string, diff --git a/packages/platform-apple/src/runner/runner-artifact.ts b/packages/platform-apple/src/runner/runner-artifact.ts index 5ed1d81680..a7dccd3c5d 100644 --- a/packages/platform-apple/src/runner/runner-artifact.ts +++ b/packages/platform-apple/src/runner/runner-artifact.ts @@ -7,13 +7,14 @@ import { runCmdStreaming, type ExecBackgroundResult, withKeyedLock, + withProcessLock, emitRequestProgress, findProjectRoot, } from './host.ts'; import type { DeviceInfo } from '@agent-device/kernel/device'; import { resolveRunnerBuildFailureHint } from './runner-contract.ts'; import { logChunk } from './runner-io.ts'; -import { acquireXcodebuildSimulatorSetRedirect } from './runner-device-set.ts'; +import { withXcodebuildSimulatorSetRedirect } from './runner-device-set.ts'; import { acquireRunnerXctestrunCacheLock, assertSafeDerivedCleanup, @@ -100,19 +101,18 @@ export async function ensureXctestrunArtifact( ); const derived = resolveRunnerDerivedPath(device, expectedCacheMetadata); return await withKeyedLock(runnerXctestrunBuildLocks, derived, async () => { - const releaseCacheLock = await acquireRunnerXctestrunCacheLock(derived); - try { - return await ensureXctestrunUnderCacheLock({ - device, - options, - projectRoot, - expectedCacheMetadata, - derived, - forceRebuild: options.forceRunnerXctestrunRebuild === true, - }); - } finally { - await releaseCacheLock(); - } + return await withProcessLock({ + acquire: () => acquireRunnerXctestrunCacheLock(derived), + task: () => + ensureXctestrunUnderCacheLock({ + device, + options, + projectRoot, + expectedCacheMetadata, + derived, + forceRebuild: options.forceRunnerXctestrunRebuild === true, + }), + }); }); } @@ -471,60 +471,59 @@ async function buildRunnerXctestrun( const provisioningArgs = device.kind === 'device' ? ['-allowProvisioningUpdates'] : []; const performanceBuildSettings = resolveRunnerPerformanceBuildSettings(); const sandboxBuildArgs = resolveRunnerSandboxBuildArgs(); - const simulatorSetRedirect = await acquireXcodebuildSimulatorSetRedirect(device); - try { - await runCmdStreaming( - 'xcodebuild', - [ - 'build-for-testing', - '-project', - projectPath, - '-scheme', - 'AgentDeviceRunner', - '-parallel-testing-enabled', - 'NO', - resolveRunnerMaxConcurrentDestinationsFlag(device), - '1', - '-destination', - resolveRunnerBuildDestination(device), - '-derivedDataPath', - derived, - ...performanceBuildSettings, - ...sandboxBuildArgs, - ...runnerBundleBuildSettings, - ...provisioningArgs, - ...signingBuildSettings, - ], - { - detached: true, - timeoutMs: buildTimeoutMs, - signal: options.budget?.signal, - onSpawn: (child) => { - runnerPrepProcesses.add(child); - child.on('close', () => { - runnerPrepProcesses.delete(child); - }); - }, - onStdoutChunk: (chunk) => { - logChunk(chunk, options.logPath, options.traceLogPath, options.verbose); - }, - onStderrChunk: (chunk) => { - logChunk(chunk, options.logPath, options.traceLogPath, options.verbose); + await withXcodebuildSimulatorSetRedirect(device, async () => { + try { + await runCmdStreaming( + 'xcodebuild', + [ + 'build-for-testing', + '-project', + projectPath, + '-scheme', + 'AgentDeviceRunner', + '-parallel-testing-enabled', + 'NO', + resolveRunnerMaxConcurrentDestinationsFlag(device), + '1', + '-destination', + resolveRunnerBuildDestination(device), + '-derivedDataPath', + derived, + ...performanceBuildSettings, + ...sandboxBuildArgs, + ...runnerBundleBuildSettings, + ...provisioningArgs, + ...signingBuildSettings, + ], + { + detached: true, + timeoutMs: buildTimeoutMs, + signal: options.budget?.signal, + onSpawn: (child) => { + runnerPrepProcesses.add(child); + child.on('close', () => { + runnerPrepProcesses.delete(child); + }); + }, + onStdoutChunk: (chunk) => { + logChunk(chunk, options.logPath, options.traceLogPath, options.verbose); + }, + onStderrChunk: (chunk) => { + logChunk(chunk, options.logPath, options.traceLogPath, options.verbose); + }, }, - }, - ); - } catch (error) { - if (isRequestCanceledError(error)) throw error; - const appErr = - error instanceof AppError ? error : new AppError('COMMAND_FAILED', String(error)); - const hint = resolveRunnerBuildFailureHint(appErr); - throw new AppError('COMMAND_FAILED', 'xcodebuild build-for-testing failed', { - error: appErr.message, - details: appErr.details, - logPath: options.logPath, - hint, - }); - } finally { - await simulatorSetRedirect?.release(); - } + ); + } catch (error) { + if (isRequestCanceledError(error)) throw error; + const appErr = + error instanceof AppError ? error : new AppError('COMMAND_FAILED', String(error)); + const hint = resolveRunnerBuildFailureHint(appErr); + throw new AppError('COMMAND_FAILED', 'xcodebuild build-for-testing failed', { + error: appErr.message, + details: appErr.details, + logPath: options.logPath, + hint, + }); + } + }); } diff --git a/packages/platform-apple/src/runner/runner-cache.ts b/packages/platform-apple/src/runner/runner-cache.ts index 966d49670d..47f188ac11 100644 --- a/packages/platform-apple/src/runner/runner-cache.ts +++ b/packages/platform-apple/src/runner/runner-cache.ts @@ -5,6 +5,7 @@ import { emitDiagnostic, readProcessStartTime, acquireProcessLock, + withProcessLock, type ProcessLockOwner, isEnvTruthy, findProjectRoot, @@ -101,18 +102,18 @@ export async function markRunnerXctestrunArtifactBadForRun( } badRunnerArtifactsForRun.add(artifact.derived); - const releaseCacheLock = await acquireRunnerXctestrunCacheLock(artifact.derived); - try { - emitRunnerXctestrunDecision('clean', 'bad_artifact', { - derived: artifact.derived, - xctestrunPath: artifact.xctestrunPath, - reason, - }); - assertSafeDerivedCleanup(artifact.derived); - cleanRunnerDerivedArtifacts(artifact.derived); - } finally { - await releaseCacheLock(); - } + await withProcessLock({ + acquire: () => acquireRunnerXctestrunCacheLock(artifact.derived), + task: async () => { + emitRunnerXctestrunDecision('clean', 'bad_artifact', { + derived: artifact.derived, + xctestrunPath: artifact.xctestrunPath, + reason, + }); + assertSafeDerivedCleanup(artifact.derived); + cleanRunnerDerivedArtifacts(artifact.derived); + }, + }); } export async function acquireRunnerXctestrunCacheLock( diff --git a/packages/platform-apple/src/runner/runner-device-set.ts b/packages/platform-apple/src/runner/runner-device-set.ts index ab95ce021a..0413f0176c 100644 --- a/packages/platform-apple/src/runner/runner-device-set.ts +++ b/packages/platform-apple/src/runner/runner-device-set.ts @@ -8,6 +8,7 @@ import { emitDiagnostic, readProcessStartTime, acquireProcessLock, + withProcessLock, type ProcessLockOwner, } from './host.ts'; @@ -18,7 +19,7 @@ const XCTEST_DEVICE_SET_LOCK_TIMEOUT_MS = 30_000; const XCTEST_DEVICE_SET_LOCK_POLL_MS = 100; const XCTEST_DEVICE_SET_LOCK_OWNER_GRACE_MS = 5_000; -type XcodebuildSimulatorSetRedirectHandle = { +export type XcodebuildSimulatorSetRedirectHandle = { release: () => Promise; }; @@ -45,6 +46,37 @@ function resolveXcodebuildSimulatorDeviceSetBackupPath( return `${xctestDeviceSetPath}${XCTEST_DEVICE_SET_BACKUP_SUFFIX}`; } +/** + * Runs `task` with the XCTest device set redirected at this simulator's device set, and gives the + * redirect back on every path out. The task is what owns the redirect's lifetime here, so a build + * that failed keeps its own error and the lock it could not hand back goes to the stale-clear path + * instead of becoming the reportable failure. + */ +export async function withXcodebuildSimulatorSetRedirect( + device: DeviceInfo, + task: () => Promise, + options: XcodebuildSimulatorSetRedirectOptions = {}, +): Promise { + const redirect = await acquireXcodebuildSimulatorSetRedirect(device, options); + if (!redirect) return await 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 { + 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 = {}, diff --git a/packages/platform-apple/src/runner/runner-disposal.ts b/packages/platform-apple/src/runner/runner-disposal.ts index 605abc1c3b..03c8c50e7d 100644 --- a/packages/platform-apple/src/runner/runner-disposal.ts +++ b/packages/platform-apple/src/runner/runner-disposal.ts @@ -11,6 +11,7 @@ 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 { @@ -191,7 +192,7 @@ async function cleanupRunnerSessionResources( await settleOwnedRunnerDeviceState(session, options); cleanupTempFile(session.xctestrunPath); cleanupTempFile(session.jsonPath); - await session.simulatorSetRedirect?.release(); + await releaseXcodebuildSimulatorSetRedirectBestEffort(session.simulatorSetRedirect); } /** diff --git a/packages/platform-apple/src/runner/runner-lease.ts b/packages/platform-apple/src/runner/runner-lease.ts index 082df145d4..20d857491c 100644 --- a/packages/platform-apple/src/runner/runner-lease.ts +++ b/packages/platform-apple/src/runner/runner-lease.ts @@ -6,6 +6,7 @@ import { emitDiagnostic, publishFileSync, acquireProcessLock, + withProcessLock, hasDeviceClaimAuthority, isProcessAlive, readProcessCommand, @@ -115,28 +116,22 @@ export function buildRunnerLease(params: { } export async function withRunnerLeaseLock(deviceId: string, task: () => Promise): Promise { - const release = await acquireProcessLock({ - lockDirPath: `${resolveRunnerLeasePath(deviceId)}.lock`, - owner: { - pid: RUNNER_OWNER_PID, - startTime: runnerOwnerStartTime(), - acquiredAtMs: Date.now(), - }, - timeoutMs: RUNNER_LEASE_LOCK_TIMEOUT_MS, - pollMs: RUNNER_LEASE_LOCK_POLL_MS, - ownerGraceMs: RUNNER_LEASE_OWNER_GRACE_MS, - description: `iOS runner lease for ${deviceId}`, + return await withProcessLock({ + acquire: () => + acquireProcessLock({ + lockDirPath: `${resolveRunnerLeasePath(deviceId)}.lock`, + owner: { + pid: RUNNER_OWNER_PID, + startTime: runnerOwnerStartTime(), + acquiredAtMs: Date.now(), + }, + timeoutMs: RUNNER_LEASE_LOCK_TIMEOUT_MS, + pollMs: RUNNER_LEASE_LOCK_POLL_MS, + ownerGraceMs: RUNNER_LEASE_OWNER_GRACE_MS, + description: `iOS runner lease for ${deviceId}`, + }), + task, }); - try { - const result = await task(); - await release(); - return result; - } catch (error) { - // A task that failed is the reportable fact; an unverified release only says the - // lease lock is still standing, which the stale-clear path resolves on its own. - await release().catch(() => undefined); - throw error; - } } function readRunnerLease(deviceId: string): RunnerLease | null { diff --git a/packages/platform-apple/src/runner/runner-session.ts b/packages/platform-apple/src/runner/runner-session.ts index 0222d514ca..7d08db6fee 100644 --- a/packages/platform-apple/src/runner/runner-session.ts +++ b/packages/platform-apple/src/runner/runner-session.ts @@ -14,6 +14,7 @@ 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 +249,7 @@ async function startRunnerSessionWithLease( }), ); } catch (error) { - await simulatorSetRedirect?.release(); + await releaseXcodebuildSimulatorSetRedirectBestEffort(simulatorSetRedirect); throw error; } const sessionId = buildRunnerSessionId(device.id, port); diff --git a/packages/platform-web/src/agent-browser-tool.test.ts b/packages/platform-web/src/agent-browser-tool.test.ts index ab257c8b67..42c09dab01 100644 --- a/packages/platform-web/src/agent-browser-tool.test.ts +++ b/packages/platform-web/src/agent-browser-tool.test.ts @@ -240,6 +240,41 @@ test('managed agent-browser setup reports an install that produced no entry', as } }); +test('managed agent-browser setup gives the install lock back on every path out', async () => { + const stateDir = mkdtempForTestSync('agent-device-web-setup-lock-'); + vi.stubEnv('npm_execpath', writeFakeNpmCliScript(stateDir)); + try { + await withNodeRuntime({ version: '24.13.0' }, async () => { + await withCommandExecutorOverride( + async (_cmd: string, args: string[]) => { + if (args.includes('install') && args.includes('--prefix')) { + writeFakeManagedAgentBrowserPackage(stateDir); + } + return { stdout: '', stderr: '', exitCode: 0 }; + }, + async () => { + await setupManagedAgentBrowser({ stateDir }); + // The package exists by now, so this second call returns from *inside* the lock + // rather than running to the end of it. + await setupManagedAgentBrowser({ stateDir }); + }, + ); + }); + } finally { + vi.unstubAllEnvs(); + assert.deepEqual(lockPathsUnder(stateDir), []); + fs.rmSync(stateDir, { recursive: true, force: true }); + } +}); + +function lockPathsUnder(directory: string): string[] { + return fs.readdirSync(directory, { withFileTypes: true }).flatMap((entry) => { + const entryPath = path.join(directory, entry.name); + if (!entry.isDirectory()) return []; + return [...(entry.name.endsWith('.lock') ? [entryPath] : []), ...lockPathsUnder(entryPath)]; + }); +} + function expectedMissingInstallHint(): string { const nodeMajor = Number.parseInt(process.versions.node.split('.')[0] ?? '0', 10); if (nodeMajor < 24) { diff --git a/packages/platform-web/src/agent-browser-tool.ts b/packages/platform-web/src/agent-browser-tool.ts index 01eaed5ffe..ec9dcb9e07 100644 --- a/packages/platform-web/src/agent-browser-tool.ts +++ b/packages/platform-web/src/agent-browser-tool.ts @@ -1,7 +1,7 @@ import crypto from 'node:crypto'; import path from 'node:path'; import { runCmd, type ExecResult } from '@agent-device/host-kit/command'; -import { acquireProcessLock } from '@agent-device/host-kit/file'; +import { acquireProcessLock, withProcessLock } from '@agent-device/host-kit/file'; import { createHostDirectoryLinkSync, ensureHostDirectorySync, @@ -70,46 +70,42 @@ export async function setupManagedAgentBrowser(options: { assertWebNodeSupported(status.nodeMajor); const processId = hostProcessId(); - const release = await acquireProcessLock({ - lockDirPath: path.join(status.installDir, '..', '.agent-browser-install.lock'), - owner: { - pid: processId, - startTime: readProcessStartTime(processId), - acquiredAtMs: Date.now(), + return await withProcessLock({ + acquire: () => + acquireProcessLock({ + lockDirPath: path.join(status.installDir, '..', '.agent-browser-install.lock'), + owner: { + pid: processId, + startTime: readProcessStartTime(processId), + acquiredAtMs: Date.now(), + }, + timeoutMs: SETUP_TIMEOUT_MS, + description: 'managed agent-browser setup', + }), + task: async () => { + const freshStatus = getManagedAgentBrowserStatus(options); + if (freshStatus.installed) return freshStatus; + ensureHostDirectorySync(freshStatus.installDir); + await installManagedAgentBrowserPackage({ + packageRoot: path.join(freshStatus.installDir, 'package'), + packageSpec: `${AGENT_BROWSER}@${MANAGED_AGENT_BROWSER_VERSION}`, + timeoutMs: SETUP_TIMEOUT_MS, + }); + // The backend entry only exists once npm has written the package. + const installedStatus = getManagedAgentBrowserStatus(options); + if (!installedStatus.entryScript) throw unusableInstallError(installedStatus); + await spawnManagedAgentBrowser(installedStatus, ['install'], { timeoutMs: SETUP_TIMEOUT_MS }); + await spawnManagedAgentBrowser(installedStatus, ['doctor', '--offline', '--quick'], { + timeoutMs: DOCTOR_TIMEOUT_MS, + }); + writeManagedAgentBrowserManifest({ + installDir: installedStatus.installDir, + packageName: AGENT_BROWSER, + version: MANAGED_AGENT_BROWSER_VERSION, + }); + return await getManagedAgentBrowserStatus(options); }, - timeoutMs: SETUP_TIMEOUT_MS, - description: 'managed agent-browser setup', }); - try { - const freshStatus = getManagedAgentBrowserStatus(options); - if (freshStatus.installed) return freshStatus; - ensureHostDirectorySync(freshStatus.installDir); - await installManagedAgentBrowserPackage({ - packageRoot: path.join(freshStatus.installDir, 'package'), - packageSpec: `${AGENT_BROWSER}@${MANAGED_AGENT_BROWSER_VERSION}`, - timeoutMs: SETUP_TIMEOUT_MS, - }); - // The backend entry only exists once npm has written the package. - const installedStatus = getManagedAgentBrowserStatus(options); - if (!installedStatus.entryScript) throw unusableInstallError(installedStatus); - await spawnManagedAgentBrowser(installedStatus, ['install'], { timeoutMs: SETUP_TIMEOUT_MS }); - await spawnManagedAgentBrowser(installedStatus, ['doctor', '--offline', '--quick'], { - timeoutMs: DOCTOR_TIMEOUT_MS, - }); - writeManagedAgentBrowserManifest({ - installDir: installedStatus.installDir, - packageName: AGENT_BROWSER, - version: MANAGED_AGENT_BROWSER_VERSION, - }); - const status = await getManagedAgentBrowserStatus(options); - await release(); - return status; - } catch (error) { - // A failed install is the reportable fact; an unverified release leaves the lock to - // the stale-clear path. - await release().catch(() => undefined); - throw error; - } } export async function doctorManagedAgentBrowser(options: { diff --git a/src/daemon/__tests__/atomic-publish-ownership.test.ts b/src/daemon/__tests__/atomic-publish-ownership.test.ts index 804f93ba9f..219e548b5e 100644 --- a/src/daemon/__tests__/atomic-publish-ownership.test.ts +++ b/src/daemon/__tests__/atomic-publish-ownership.test.ts @@ -34,10 +34,13 @@ test('the process lock publishes its owner record without publishing files by ha assert.doesNotMatch(source, /fs\.writeFileSync\s*\(/); }); -test('the process lock renames only between the lock path and its reclaimed name', () => { +// A reclaim that parked the judged directory under another name put the lock path in the state +// a polling contender reads as free, so nothing here may rename it. What it does instead is +// empty and remove the path, which cannot address anything but the directory judged stale. +test('the process lock reclaims in place instead of renaming the lock path', () => { const source = fs.readFileSync(PROCESS_LOCK_SOURCE, 'utf8'); - const renamed = [...source.matchAll(/fs\.renameSync\(([^)]*)\)/g)].map((match) => match[1]); - assert.deepEqual(renamed.sort(), ['asidePath, lockDirPath', 'lockDirPath, asidePath']); + assert.doesNotMatch(source, /renameSync|asidePath|\.reclaimed-/); + assert.match(source, /fs\.rmdirSync/); }); test('durable publishers share the host-kit durable publication owner', () => { diff --git a/src/daemon/device-claim-store.ts b/src/daemon/device-claim-store.ts index 53d5dac240..fb404e3a4a 100644 --- a/src/daemon/device-claim-store.ts +++ b/src/daemon/device-claim-store.ts @@ -1,6 +1,6 @@ import fs from 'node:fs'; import path from 'node:path'; -import { publishFileSync, acquireProcessLock } from '@agent-device/host-kit/file'; +import { publishFileSync, acquireProcessLock, withProcessLock } from '@agent-device/host-kit/file'; import { readCurrentOwnerIdentity } from '@agent-device/host-kit/process'; import { resolveDeviceClaimPath } from './device-claim-paths.ts'; @@ -25,20 +25,14 @@ export async function withDeviceClaimLock( task: () => Promise, ): Promise { const owner = readCurrentOwnerIdentity(); - const release = await acquireProcessLock({ - lockDirPath: `${resolveDeviceClaimPath(deviceKey)}.lock`, - owner: { pid: owner.pid, startTime: owner.startTime, acquiredAtMs: Date.now() }, - timeoutMs: DEVICE_CLAIM_LOCK_TIMEOUT_MS, - description: `device claim for ${deviceKey}`, + return await withProcessLock({ + acquire: () => + acquireProcessLock({ + lockDirPath: `${resolveDeviceClaimPath(deviceKey)}.lock`, + owner: { pid: owner.pid, startTime: owner.startTime, acquiredAtMs: Date.now() }, + timeoutMs: DEVICE_CLAIM_LOCK_TIMEOUT_MS, + description: `device claim for ${deviceKey}`, + }), + task, }); - try { - const result = await task(); - await release(); - return result; - } catch (error) { - // A task that failed is the reportable fact; an unverified release only says the - // lock is still standing, which the stale-clear path resolves on its own. - await release().catch(() => undefined); - throw error; - } } From 17ca384852e0b170ecbde70f2fa90fe11ecc41a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 10:28:55 +0200 Subject: [PATCH 05/18] fix(host-kit): a spent claim is dead to the next reclaim from this process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A release that could not verify ownership — the `unlink` of the record refused by EACCES or EMFILE, the record unreadable — left the lock directory standing with a record naming this live pid. The next acquire from the same process read that record, found a live owner, and waited 30 s for it; the runner build or launch behind it failed with "Timed out waiting for ...". The only thing that would end the wait was restarting the daemon, because the pid and start time the reclaim reads outlive the claim that was written with them. So the claim, not the process, is what the reclaim has to date. A token is issued per acquisition, and the moment a release is asked for, nobody inside this process is acting on it — whether the removal afterwards succeeds or not. `clearStaleProcessLock` now reads a record naming this pid under a token this process no longer holds as the dead claim it is, and takes the path back instead of waiting for a restart nothing is going to perform. The failed release is recorded as `process_lock_release_unverified` rather than vanishing into the caller that swallowed it. Two doubles that pretended to be another process were naming this pid with a token it never issued, which is now precisely the shape that says "spent, not rival"; they name `process.ppid` — another live process, which is what a contender has to be. The mutation that turns the new test red is deleting the spent-claim disjunct from the reclaim decision: the second acquire then waits out its whole timeout exactly as the report describes. --- CHANGELOG.md | 6 ++ .../src/internal/process-lock.test.ts | 62 +++++++++++++++++-- .../host-kit/src/internal/process-lock.ts | 49 +++++++++++++-- 3 files changed, 107 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba3e951e30..662cb88e4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -147,6 +147,12 @@ against the instant that attempt signalled the recorder rather than the time of the retry. A stop that fails and is then abandoned keeps the pulled set on the host beside `--out`. Chunk paths and `--client-output-path` naming are unchanged. +- Fixed: a process lock that could not be given back no longer waits for the daemon to restart. A + release that cannot verify ownership — a refused `unlink`, an unreadable record — left its record + standing and naming the live daemon, and the next acquire read that as a live owner and timed out + after 30 s on every runner build or launch until the process restarted. The claim inside is spent + the moment the release is asked for, so a reclaim here now reads it as dead and takes the path + back, and the failed release is recorded in the request log as `process_lock_release_unverified`. - Changed (sessions): the implicit session is now keyed by workspace **and platform**, so one checkout can drive iOS and Android without inventing a `--session` name for every command (#2580). An diff --git a/packages/host-kit/src/internal/process-lock.test.ts b/packages/host-kit/src/internal/process-lock.test.ts index 4542225bf4..e132f28d20 100644 --- a/packages/host-kit/src/internal/process-lock.test.ts +++ b/packages/host-kit/src/internal/process-lock.test.ts @@ -365,6 +365,47 @@ test('release reports a lock whose owner record it cannot read instead of cleari assert.equal(fs.existsSync(lockDirPath), true); }); +// A release that cannot verify ownership leaves its record standing under a claim this process has +// spent. The pid inside that record is this live process, so a reclaim that reads only the pid and +// its start time waits for a restart nothing is going to perform while every contender in here +// times out on a lock that is already free. +test('a release that could not verify ownership does not wedge the next acquire from this process', async () => { + const lockDirPath = path.join(tmpDir, 'spent-claim.lock'); + const ownerFilePath = path.join(lockDirPath, 'owner.json'); + const release = await acquireProcessLock({ lockDirPath, owner: currentProcessOwner() }); + + // The unlink the release needs is refused, which is what an EACCES or EMFILE looks like here. + const realUnlink = fs.unlinkSync; + const unlinkSpy = vi.spyOn(fs, 'unlinkSync').mockImplementation(((target: fs.PathLike) => { + if (String(target) === ownerFilePath) { + throw Object.assign(new Error('EACCES: permission denied, unlink'), { code: 'EACCES' }); + } + return realUnlink(target as string); + }) as typeof fs.unlinkSync); + try { + await assert.rejects( + () => release(), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.ownerReleaseUnverified, true); + return true; + }, + ); + } finally { + unlinkSpy.mockRestore(); + } + assert.equal(fs.existsSync(ownerFilePath), true); + + const next = await acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + timeoutMs: 1_000, + pollMs: 5, + }); + await next(); + assert.equal(fs.existsSync(lockDirPath), false); +}); + test('acquireProcessLock reclaims a stray path in place of the lock directory', async () => { const lockDirPath = path.join(tmpDir, 'stray.lock'); fs.writeFileSync(lockDirPath, 'not a lock'); @@ -409,7 +450,14 @@ test('a contender that claims the path during a reclaim keeps its lock', async ( fs.mkdirSync(lockDirPath); fs.writeFileSync( ownerFilePath, - JSON.stringify({ ...currentProcessOwner(), claimToken: 'contender-claim' }), + // A contender is another live process, and the pid has to say so: a record naming this + // process with a token this process never issued is a spent claim, not a rival. + JSON.stringify({ + pid: process.ppid, + startTime: null, + acquiredAtMs: Date.now(), + claimToken: 'contender-claim', + }), ); return realMkdir(target as string, options as fs.MakeDirectoryOptions); }) as typeof fs.mkdirSync); @@ -426,7 +474,7 @@ test('a contender that claims the path during a reclaim keeps its lock', async ( (error: unknown) => { assert.ok(error instanceof AppError); assert.equal(error.details?.ownerLiveness, 'live'); - assert.equal(error.details?.ownerPid, process.pid); + assert.equal(error.details?.ownerPid, process.ppid); return true; }, ); @@ -435,7 +483,7 @@ test('a contender that claims the path during a reclaim keeps its lock', async ( pid: number; claimToken: string; }; - assert.equal(record.pid, process.pid); + assert.equal(record.pid, process.ppid); assert.equal(record.claimToken, 'contender-claim'); } finally { mkdirSpy.mockRestore(); @@ -466,7 +514,13 @@ test('a claim published while a reclaim holds the mutex outlives the empty direc published = true; fs.writeFileSync( ownerFilePath, - JSON.stringify({ ...currentProcessOwner(), claimToken: 'late-claim' }), + // See the contender above: another process's claim names another pid. + JSON.stringify({ + pid: process.ppid, + startTime: null, + acquiredAtMs: Date.now(), + claimToken: 'late-claim', + }), ); return realMkdir(target as string, options as fs.MakeDirectoryOptions); }) as typeof fs.mkdirSync); diff --git a/packages/host-kit/src/internal/process-lock.ts b/packages/host-kit/src/internal/process-lock.ts index 942dd87dc9..3a716ae2af 100644 --- a/packages/host-kit/src/internal/process-lock.ts +++ b/packages/host-kit/src/internal/process-lock.ts @@ -3,6 +3,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { AppError } from '@agent-device/kernel/errors'; import { publishFileSync } from './atomic-file.ts'; +import { emitDiagnostic } from './diagnostics.ts'; import { classifyOwnerLiveness, ownerIdentityMatches } from './owner-identity.ts'; import { sleep } from './timeouts.ts'; @@ -36,9 +37,10 @@ export type ProcessLockRelease = () => Promise; * caller otherwise answers by hand: which of two failures to report. * * A task that failed is the reportable fact, and an unverified release afterwards only says the - * lock is still standing, which the stale-clear path resolves on its own. On the success path - * the release is not best effort: a lock this process could not give back is not a completed - * task, and swallowing it would report success while the next contender waits. + * lock is still standing under a claim this process has spent, which the next reclaim here reads + * as dead. On the success path the release is not best effort: a lock this process could not give + * back is not a completed task, and swallowing it would report success while the next contender + * waits. */ export async function withProcessLock(params: { acquire: () => Promise; @@ -60,6 +62,16 @@ type ProcessLockOwnerReading = | { kind: 'unwritten' } | { kind: 'unreadable' }; +/** + * The claims this process is holding right now, by token. A record naming this pid is not + * evidence that this process holds the lock: a release that could not verify ownership leaves its + * record standing, and a handle dropped without a release does too. Both name a claim nobody here + * is acting on, and only a token absent from this set can say so — the pid and start time outlive + * the claim, so a reclaim that waited on those would wait until this process restarts while every + * contender inside it times out on a lock that is already free. + */ +const liveClaimTokens = new Set(); + export async function acquireProcessLock(params: { lockDirPath: string; owner: ProcessLockOwner; @@ -76,15 +88,19 @@ export async function acquireProcessLock(params: { const description = params.description ?? 'process lock'; fs.mkdirSync(path.dirname(lockDirPath), { recursive: true }); - const claim: ProcessLockOwnerRecord = { ...owner, claimToken: crypto.randomUUID() }; + const claimToken = crypto.randomUUID(); + const claim: ProcessLockOwnerRecord = { ...owner, claimToken }; while (Date.now() < deadline) { try { fs.mkdirSync(lockDirPath); writeProcessLockOwner(ownerFilePath, claim); + liveClaimTokens.add(claimToken); let released = false; return async () => { if (released) return; + // Asking to give the lock back ends the claim, whatever the removal below concludes. + liveClaimTokens.delete(claimToken); const outcome = releaseProcessLock(lockDirPath, ownerFilePath, claim); if (outcome !== 'unverified') { released = true; @@ -92,6 +108,15 @@ export async function acquireProcessLock(params: { } // The record still names us as far as we can tell and we could not read far // enough to be sure, so the lock stays in place and the caller hears why. + emitDiagnostic({ + level: 'warn', + phase: 'process_lock_release_unverified', + data: { + lockDirPath, + description, + ownerReleaseUnverified: true, + }, + }); throw new AppError('COMMAND_FAILED', `Cannot verify ownership of ${description}`, { lockDirPath, ownerReleaseUnverified: true, @@ -193,9 +218,10 @@ function clearStaleProcessLock( const reading = readProcessLockOwner(ownerFilePath); if (reading.kind === 'owner') { // A record identifies the acquisition that wrote it, so the directory around a claim judged - // dead is that claim's property. + // dead is that claim's property. The claim can be dead while the process that wrote it is + // live, which is what the token says and the pid cannot. return ( - !isLiveProcessLockOwner(reading.owner) && + (!isLiveProcessLockOwner(reading.owner) || isSpentOwnClaim(reading.owner)) && reclaimLockUnderMutex(lockDirPath, ownerFilePath, ownerGraceMs, { kind: 'dead-claim', claimToken: reading.owner.claimToken, @@ -440,3 +466,14 @@ function isLiveProcessLockOwner(owner: ProcessLockOwner): boolean { const liveness = classifyOwnerLiveness({ owner }); return liveness !== 'owner-process-dead' && liveness !== 'owner-process-reused'; } + +/** + * This process wrote the record and nothing inside it is acting on that claim any more: a release + * that could not verify ownership left it standing, or a handle was dropped without one. Waiting + * for the pid would be waiting for this process to restart. + */ +function isSpentOwnClaim(owner: ProcessLockOwnerRecord): boolean { + return ( + owner.pid === process.pid && owner.claimToken !== null && !liveClaimTokens.has(owner.claimToken) + ); +} From 805ddd2dba2f3c425b3686829eabbbe896ef8a52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 10:28:55 +0200 Subject: [PATCH 06/18] fix(apple-runner): a redirect's best-effort give-back forgives only its own release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../__tests__/runner-device-set.test.ts | 46 +++++++++++++- .../runner-request-cancellation.test.ts | 1 + .../runner-session-speculative.test.ts | 9 ++- .../runner-session-stale-bundles.test.ts | 5 +- .../runner/__tests__/runner-session.test.ts | 10 ++- .../src/runner/runner-device-set.ts | 61 +++++++++++-------- .../src/runner/runner-disposal.ts | 3 +- .../src/runner/runner-session-types.ts | 6 +- .../src/runner/runner-session.ts | 3 +- 9 files changed, 104 insertions(+), 40 deletions(-) diff --git a/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts index f7226c49da..48c2c2d448 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts +++ b/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(); + } + }); +}); diff --git a/packages/platform-apple/src/runner/__tests__/runner-request-cancellation.test.ts b/packages/platform-apple/src/runner/__tests__/runner-request-cancellation.test.ts index c6f0f45932..8b364e3441 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-request-cancellation.test.ts +++ b/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: '' }); diff --git a/packages/platform-apple/src/runner/__tests__/runner-session-speculative.test.ts b/packages/platform-apple/src/runner/__tests__/runner-session-speculative.test.ts index 8f0691988e..8f862ac73a 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-session-speculative.test.ts +++ b/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, {}); diff --git a/packages/platform-apple/src/runner/__tests__/runner-session-stale-bundles.test.ts b/packages/platform-apple/src/runner/__tests__/runner-session-stale-bundles.test.ts index d53ebc43aa..c2385b8f39 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-session-stale-bundles.test.ts +++ b/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); diff --git a/packages/platform-apple/src/runner/__tests__/runner-session.test.ts b/packages/platform-apple/src/runner/__tests__/runner-session.test.ts index 3b1596492d..154c950eba 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-session.test.ts +++ b/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 })); diff --git a/packages/platform-apple/src/runner/runner-device-set.ts b/packages/platform-apple/src/runner/runner-device-set.ts index 0413f0176c..f93000ab61 100644 --- a/packages/platform-apple/src/runner/runner-device-set.ts +++ b/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; + /** + * 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; }; type XcodebuildSimulatorSetRedirectOptions = { @@ -62,21 +70,6 @@ export async function withXcodebuildSimulatorSetRedirect( 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 { - 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; diff --git a/packages/platform-apple/src/runner/runner-disposal.ts b/packages/platform-apple/src/runner/runner-disposal.ts index 03c8c50e7d..20c3694713 100644 --- a/packages/platform-apple/src/runner/runner-disposal.ts +++ b/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(); } /** diff --git a/packages/platform-apple/src/runner/runner-session-types.ts b/packages/platform-apple/src/runner/runner-session-types.ts index 7e067057bc..25b58cdef3 100644 --- a/packages/platform-apple/src/runner/runner-session-types.ts +++ b/packages/platform-apple/src/runner/runner-session-types.ts @@ -50,7 +50,11 @@ export type RunnerSession = { startupTimings?: Record; startupTimingsReported?: boolean; logicalLeaseContext?: RunnerLogicalLeaseContext; - simulatorSetRedirect?: { release: () => Promise }; + /** `XcodebuildSimulatorSetRedirectHandle`, seen through the two operations a session performs. */ + simulatorSetRedirect?: { + release: () => Promise; + releaseBestEffort: () => Promise; + }; lease?: RunnerLease; }; diff --git a/packages/platform-apple/src/runner/runner-session.ts b/packages/platform-apple/src/runner/runner-session.ts index 7d08db6fee..6c77ca4a8a 100644 --- a/packages/platform-apple/src/runner/runner-session.ts +++ b/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 { @@ -249,7 +248,7 @@ async function startRunnerSessionWithLease( }), ); } catch (error) { - await releaseXcodebuildSimulatorSetRedirectBestEffort(simulatorSetRedirect); + await simulatorSetRedirect?.releaseBestEffort(); throw error; } const sessionId = buildRunnerSessionId(device.id, port); From 63ab4b2d07689583b1093ddb6f55de5ce8fb5ba7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 12:54:19 +0200 Subject: [PATCH 07/18] test(apple-runner): one device-set redirect double for the session tests The size ratchet refused this PR's six-line growth in `runner-session.test.ts`, which is 1,955 lines at the merge-base and already over the tripwire, and its own remedy is the right one here: the handle the launch hands the session was being fabricated four times over, twice in that file alone, with a per-file spy that each file then counted calls on. It now lives once in `runner-session-fixtures.ts`, the module this test family already shares, and the family is five lines shorter than it was. One spy answers both give-backs on purpose. The session-level tests ask whether the host's device set came back when a launch failed or a session was disposed; which of the two doors it came back through is the thing this PR changed, and that is pinned where the handle is made, in `runner-device-set.test.ts`, where each door is a separate test. --- .../runner-request-cancellation.test.ts | 8 ++----- .../__tests__/runner-session-fixtures.ts | 13 +++++++++++ .../runner-session-speculative.test.ts | 12 ++++------ .../runner-session-stale-bundles.test.ts | 8 ++----- .../runner/__tests__/runner-session.test.ts | 22 +++++++------------ 5 files changed, 29 insertions(+), 34 deletions(-) diff --git a/packages/platform-apple/src/runner/__tests__/runner-request-cancellation.test.ts b/packages/platform-apple/src/runner/__tests__/runner-request-cancellation.test.ts index 8b364e3441..300fe306a6 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-request-cancellation.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-request-cancellation.test.ts @@ -5,6 +5,7 @@ import path from 'node:path'; import { beforeEach, test, vi } from 'vitest'; import { IOS_DEVICE, IOS_SIMULATOR } from './device-fixtures.ts'; import { mkdtempForTestSync } from './tmp-dir.ts'; +import { redirectHandle } from './runner-session-fixtures.ts'; import { appleRunnerTestHost } from '../test-host.ts'; const { @@ -22,7 +23,6 @@ const { mockSignalPidsBestEffort, mockSignalProcessGroupBestEffort, mockWaitForRunner, - mockRedirectRelease, } = vi.hoisted(() => ({ mockAcquireXcodebuildSimulatorSetRedirect: vi.fn(), mockEnsureXctestrunArtifact: vi.fn(), @@ -39,7 +39,6 @@ const { mockSignalPidsBestEffort: vi.fn(), mockSignalProcessGroupBestEffort: vi.fn(), mockWaitForRunner: vi.fn(), - mockRedirectRelease: vi.fn(), })); vi.mock('../runner-io.ts', async () => { @@ -155,10 +154,7 @@ beforeEach(async () => { }); mockResolveExpectedRunnerCacheMetadata.mockReturnValue({ schemaVersion: 1 }); mockResolveRunnerDerivedPath.mockReturnValue('/tmp/derived'); - mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue({ - release: mockRedirectRelease, - releaseBestEffort: mockRedirectRelease, - }); + mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue(redirectHandle); mockRunCmdBackground.mockReturnValue(makeBackgroundRunner(4242)); mockRunAppleToolCommand.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' }); mockIsProcessAlive.mockReturnValue(true); diff --git a/packages/platform-apple/src/runner/__tests__/runner-session-fixtures.ts b/packages/platform-apple/src/runner/__tests__/runner-session-fixtures.ts index 56a192f941..2c7bc07bf6 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-session-fixtures.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-session-fixtures.ts @@ -1,10 +1,12 @@ import assert from 'node:assert/strict'; import fs from 'node:fs'; import { EventEmitter } from 'node:events'; +import { vi } from 'vitest'; import { IOS_SIMULATOR } from './device-fixtures.ts'; import { appleRunnerTestHost } from '../test-host.ts'; import { runnerOwnerStartTime, type RunnerLease } from '../runner-lease.ts'; import type { RunnerSession } from '../runner-session-types.ts'; +import type { XcodebuildSimulatorSetRedirectHandle } from '../runner-device-set.ts'; // Fabricated runner sessions, leases, background children, and transport // payloads shared by the runner-session tests. The child pids here are made up @@ -182,3 +184,14 @@ export function makeClassifyOwnerLivenessViaMocks(deps: { return stateDir ? classifyStateDir(stateDir) : 'live'; }; } + +/** + * The give-back a launched session holds. One spy answers both strictnesses: the the host's device set came back, and which door it came back + * through is pinned where the handle is made, in `runner-device-set.test.ts`. + */ +export const redirectRelease = vi.fn(async () => {}); + +export const redirectHandle: XcodebuildSimulatorSetRedirectHandle = { + release: redirectRelease, + releaseBestEffort: redirectRelease, +}; diff --git a/packages/platform-apple/src/runner/__tests__/runner-session-speculative.test.ts b/packages/platform-apple/src/runner/__tests__/runner-session-speculative.test.ts index 8f862ac73a..af8b8e16aa 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-session-speculative.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-session-speculative.test.ts @@ -6,6 +6,7 @@ import { makeBackgroundRunner, makeClassifyOwnerLivenessViaMocks, runnerResponse, + redirectHandle, } from './runner-session-fixtures.ts'; import { mkdtempForTestSync } from './tmp-dir.ts'; @@ -28,7 +29,6 @@ const { mockSignalPidsBestEffort, mockSignalProcessGroupBestEffort, mockWaitForRunner, - mockRedirectRelease, } = vi.hoisted(() => ({ mockAcquireXcodebuildSimulatorSetRedirect: vi.fn(), mockCleanupTempFile: vi.fn(), @@ -52,7 +52,6 @@ const { mockSignalPidsBestEffort: vi.fn(), mockSignalProcessGroupBestEffort: vi.fn(), mockWaitForRunner: vi.fn(), - mockRedirectRelease: vi.fn(), })); vi.mock('../runner-io.ts', async () => { @@ -138,10 +137,7 @@ beforeEach(async () => { }); mockResolveExpectedRunnerCacheMetadata.mockReturnValue({ schemaVersion: 1 }); mockResolveRunnerDerivedPath.mockReturnValue('/tmp/derived'); - mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue({ - release: mockRedirectRelease, - releaseBestEffort: mockRedirectRelease, - }); + mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue(redirectHandle); mockRunCmdBackground.mockReturnValue(makeBackgroundRunner(4242)); mockRunAppleToolCommand.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' }); mockIsProcessAlive.mockReturnValue(true); @@ -192,7 +188,7 @@ test('a release that arrives while the speculative start is still in flight stop }); mockAcquireXcodebuildSimulatorSetRedirect.mockImplementation(async () => { await gate; - return { release: mockRedirectRelease, releaseBestEffort: mockRedirectRelease }; + return redirectHandle; }); const starting = ensureRunnerSession(device, { speculative: true }); @@ -218,7 +214,7 @@ test('a release that waits out a demanded start leaves that runner alone', async }); mockAcquireXcodebuildSimulatorSetRedirect.mockImplementation(async () => { await gate; - return { release: mockRedirectRelease, releaseBestEffort: mockRedirectRelease }; + return redirectHandle; }); const starting = ensureRunnerSession(device, {}); diff --git a/packages/platform-apple/src/runner/__tests__/runner-session-stale-bundles.test.ts b/packages/platform-apple/src/runner/__tests__/runner-session-stale-bundles.test.ts index c2385b8f39..693cfa442e 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-session-stale-bundles.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-session-stale-bundles.test.ts @@ -6,6 +6,7 @@ import { makeBackgroundRunner, makeClassifyOwnerLivenessViaMocks, runnerResponse, + redirectHandle, } from './runner-session-fixtures.ts'; import { mkdtempForTestSync } from './tmp-dir.ts'; @@ -28,7 +29,6 @@ const { mockSignalPidsBestEffort, mockSignalProcessGroupBestEffort, mockWaitForRunner, - mockRedirectRelease, } = vi.hoisted(() => ({ mockAcquireXcodebuildSimulatorSetRedirect: vi.fn(), mockCleanupTempFile: vi.fn(), @@ -56,7 +56,6 @@ const { mockSignalPidsBestEffort: vi.fn(), mockSignalProcessGroupBestEffort: vi.fn(), mockWaitForRunner: vi.fn(), - mockRedirectRelease: vi.fn(), })); vi.mock('../runner-io.ts', async () => { @@ -146,10 +145,7 @@ beforeEach(async () => { }); mockResolveExpectedRunnerCacheMetadata.mockReturnValue({ schemaVersion: 1 }); mockResolveRunnerDerivedPath.mockReturnValue('/tmp/derived'); - mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue({ - release: mockRedirectRelease, - releaseBestEffort: mockRedirectRelease, - }); + mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue(redirectHandle); mockRunCmdBackground.mockReturnValue(makeBackgroundRunner(4242)); mockRunAppleToolCommand.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' }); mockIsProcessAlive.mockReturnValue(true); diff --git a/packages/platform-apple/src/runner/__tests__/runner-session.test.ts b/packages/platform-apple/src/runner/__tests__/runner-session.test.ts index 154c950eba..ca8c8aafbc 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-session.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-session.test.ts @@ -15,6 +15,8 @@ import { makeRunnerSession, runnerError, runnerResponse, + redirectHandle, + redirectRelease, } from './runner-session-fixtures.ts'; import { mkdtempForTestSync } from './tmp-dir.ts'; @@ -37,7 +39,6 @@ const { mockSignalPidsBestEffort, mockSignalProcessGroupBestEffort, mockWaitForRunner, - mockRedirectRelease, } = vi.hoisted(() => ({ mockAcquireXcodebuildSimulatorSetRedirect: vi.fn(), mockCleanupTempFile: vi.fn(), @@ -67,7 +68,6 @@ const { mockSignalPidsBestEffort: vi.fn(), mockSignalProcessGroupBestEffort: vi.fn(), mockWaitForRunner: vi.fn(), - mockRedirectRelease: vi.fn(), })); // Fixed owner-identity value shared by the readProcessStartTime override @@ -192,10 +192,7 @@ beforeEach(async () => { }); mockResolveExpectedRunnerCacheMetadata.mockReturnValue({ schemaVersion: 1 }); mockResolveRunnerDerivedPath.mockReturnValue('/tmp/derived'); - mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue({ - release: mockRedirectRelease, - releaseBestEffort: mockRedirectRelease, - }); + mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue(redirectHandle); mockRunCmdBackground.mockReturnValue(makeBackgroundRunner(4242)); mockRunAppleToolCommand.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' }); mockIsProcessAlive.mockReturnValue(true); @@ -703,10 +700,7 @@ 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, - releaseBestEffort: mockRedirectRelease, - }); + mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue(redirectHandle); mockRunCmdBackground.mockReturnValue(makeBackgroundRunner(4242)); mockWaitForRunner.mockResolvedValue(runnerResponse({ uptimeMs: 1 })); @@ -857,7 +851,7 @@ test('shutdown detach keeps scoped simulator-set runner sessions for the kill pa // XCTestDevices symlink; detach never releases the redirect itself. assert.equal(detached, 0); assert.ok(getRunnerSessionSnapshot(device.id)); - assert.equal(mockRedirectRelease.mock.calls.length, 0); + assert.equal(redirectRelease.mock.calls.length, 0); }); test('runner session startup kills legacy ownerless xcodebuild before launching a new runner', async () => { @@ -1471,7 +1465,7 @@ test('runner session restarts dead runner without graceful shutdown', async () = ['/tmp/session-runner.xctestrun'], ['/tmp/session-runner.json'], ]); - assert.equal(mockRedirectRelease.mock.calls.length, 1); + assert.equal(redirectRelease.mock.calls.length, 1); }); test('runner session stop kills only owned stale xcodebuild runner processes without in-memory session', async () => { @@ -1510,7 +1504,7 @@ test('runner session abort removes owned lease for in-memory sessions', async () ['/tmp/session-runner.xctestrun'], ['/tmp/session-runner.json'], ]); - assert.equal(mockRedirectRelease.mock.calls.length, 1); + assert.equal(redirectRelease.mock.calls.length, 1); }); function isXcodebuildPkillCall(call: unknown[]): boolean { @@ -1552,7 +1546,7 @@ test('runner session invalidation skips graceful shutdown and removes stale sess ['/tmp/session-runner.xctestrun'], ['/tmp/session-runner.json'], ]); - assert.equal(mockRedirectRelease.mock.calls.length, 1); + assert.equal(redirectRelease.mock.calls.length, 1); assert.equal(getRunnerSessionSnapshot(device.id), null); }); From 170269760cd918d045eb80d614571d2c91fbdba3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 14:44:31 +0200 Subject: [PATCH 08/18] test(apple-runner): the close tests' redirect double carries both give-backs The close-finalization and session-close tests arrived from `main` answering the device-set redirect with `{ release }`, which was the entire handle the day they were written. Disposal now asks the session for `releaseBestEffort`, so those tests failed the moment this branch met them: `session.simulatorSetRedirect?.releaseBestEffort is not a function`. The double is what is incomplete here, not the teardown; guarding the call for a handle that does not implement its own type would only teach the next caller to hand one over. Both files now take the handle from `runner-session-fixtures.ts`, the module their neighbours already use for it, and drop the per-file spy that only ever appeared in the setup. That the gap went unnoticed by typecheck is the untyped `vi.fn()` standing in for `acquireXcodebuildSimulatorSetRedirect`, which answers anything; every redirect double in the tree now references the shared handle, and the `{ release }` literals left are the type's own field and the object the owning module builds. --- .../src/runner/__tests__/runner-close-finalization.test.ts | 5 ++--- .../src/runner/__tests__/runner-session-close.test.ts | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/platform-apple/src/runner/__tests__/runner-close-finalization.test.ts b/packages/platform-apple/src/runner/__tests__/runner-close-finalization.test.ts index ae5be8bd3f..7aeb459865 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-close-finalization.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-close-finalization.test.ts @@ -14,6 +14,7 @@ import { makeRunnerLease, runnerError, runnerResponse, + redirectHandle, } from './runner-session-fixtures.ts'; import { mkdtempForTestSync } from './tmp-dir.ts'; import { bindAppleApplicationLifecycle } from '../../lifecycle.ts'; @@ -56,7 +57,6 @@ const { mockSignalPidsBestEffort, mockSignalProcessGroupBestEffort, mockWaitForRunner, - mockRedirectRelease, } = vi.hoisted(() => ({ mockAcquireXcodebuildSimulatorSetRedirect: vi.fn(), mockCleanupTempFile: vi.fn(), @@ -76,7 +76,6 @@ const { mockSignalPidsBestEffort: vi.fn(), mockSignalProcessGroupBestEffort: vi.fn(), mockWaitForRunner: vi.fn(), - mockRedirectRelease: vi.fn(), })); const TEST_OWNER_START_TIME = 'fixed-test-owner-start-time'; @@ -169,7 +168,7 @@ beforeEach(async () => { }); mockResolveExpectedRunnerCacheMetadata.mockReturnValue({ schemaVersion: 1 }); mockResolveRunnerDerivedPath.mockReturnValue('/tmp/derived'); - mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue({ release: mockRedirectRelease }); + mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue(redirectHandle); mockRunCmdBackground.mockReturnValue(makeBackgroundRunner(4242)); mockRunAppleToolCommand.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' }); mockIsProcessAlive.mockReturnValue(true); diff --git a/packages/platform-apple/src/runner/__tests__/runner-session-close.test.ts b/packages/platform-apple/src/runner/__tests__/runner-session-close.test.ts index a1fbdd8227..5010fdea26 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-session-close.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-session-close.test.ts @@ -8,6 +8,7 @@ import { makeRunnerSession, runnerError, runnerResponse, + redirectHandle, } from './runner-session-fixtures.ts'; import { mkdtempForTestSync } from './tmp-dir.ts'; @@ -35,7 +36,6 @@ const { mockSignalPidsBestEffort, mockSignalProcessGroupBestEffort, mockWaitForRunner, - mockRedirectRelease, } = vi.hoisted(() => ({ mockAcquireXcodebuildSimulatorSetRedirect: vi.fn(), mockCleanupTempFile: vi.fn(), @@ -55,7 +55,6 @@ const { mockSignalPidsBestEffort: vi.fn(), mockSignalProcessGroupBestEffort: vi.fn(), mockWaitForRunner: vi.fn(), - mockRedirectRelease: vi.fn(), })); const TEST_OWNER_START_TIME = 'fixed-test-owner-start-time'; @@ -152,7 +151,7 @@ beforeEach(async () => { }); mockResolveExpectedRunnerCacheMetadata.mockReturnValue({ schemaVersion: 1 }); mockResolveRunnerDerivedPath.mockReturnValue('/tmp/derived'); - mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue({ release: mockRedirectRelease }); + mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue(redirectHandle); mockRunCmdBackground.mockReturnValue(makeBackgroundRunner(4242)); mockRunAppleToolCommand.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' }); mockIsProcessAlive.mockReturnValue(true); From 606060dcd3cdaf3afac16ace197b17f7f589689d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 15:50:24 +0200 Subject: [PATCH 09/18] fix(host-kit): a spent claim belongs to the loading that issued it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pid identifies a process, and the spent-claim rule reads a record that names one. Two bundles of `process-lock.ts` loaded in the same process — a nested install, a vendored copy in another package's setup — share that pid and its start time, and neither can see the other's `liveClaimTokens`. The rule as written would therefore read the other copy's live claim as a spent one and clear a lock somebody is holding, which is a worse failure than the 30 s wait the rule exists to end. The record now carries which loading issued the claim, and a spent claim requires that id to be this one. A record without it — written before claims carried an issuer, or by code that never did — is not evidence of a spent claim and stays subject to the liveness answer, so nothing that reclaimed a lock before stops reclaiming it now. The field is optional in the parser rather than part of the required shape, because a record from an older daemon must still parse rather than read as unreadable. --- .../src/internal/process-lock.test.ts | 33 +++++++++++++++++++ .../host-kit/src/internal/process-lock.ts | 21 +++++++++--- 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/packages/host-kit/src/internal/process-lock.test.ts b/packages/host-kit/src/internal/process-lock.test.ts index e132f28d20..58bb487c2b 100644 --- a/packages/host-kit/src/internal/process-lock.test.ts +++ b/packages/host-kit/src/internal/process-lock.test.ts @@ -406,6 +406,39 @@ test('a release that could not verify ownership does not wedge the next acquire assert.equal(fs.existsSync(lockDirPath), false); }); +// The spent-claim rule reads a record that names this process, so it has to know which copy of this +// module wrote it. Two bundles of `process-lock.ts` in one process share the pid and the start time, +// and neither can see the other's tokens; reading the other's live claim as spent would clear a lock +// somebody is holding, which is worse than the wait the rule exists to end. +test('a claim issued by another loading of this module is not read as spent', async () => { + const lockDirPath = path.join(tmpDir, 'other-issuer.lock'); + fs.mkdirSync(lockDirPath); + fs.writeFileSync( + path.join(lockDirPath, 'owner.json'), + JSON.stringify({ + ...currentProcessOwner(), + claimToken: 'a-token-this-loading-never-issued', + claimIssuerId: 'another-loading-of-this-module', + }), + ); + + await assert.rejects( + () => + acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + timeoutMs: 50, + pollMs: 5, + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.match(error.message, /Timed out waiting for/); + return true; + }, + ); + assert.equal(fs.existsSync(path.join(lockDirPath, 'owner.json')), true); +}); + test('acquireProcessLock reclaims a stray path in place of the lock directory', async () => { const lockDirPath = path.join(tmpDir, 'stray.lock'); fs.writeFileSync(lockDirPath, 'not a lock'); diff --git a/packages/host-kit/src/internal/process-lock.ts b/packages/host-kit/src/internal/process-lock.ts index 3a716ae2af..19941445a0 100644 --- a/packages/host-kit/src/internal/process-lock.ts +++ b/packages/host-kit/src/internal/process-lock.ts @@ -27,6 +27,12 @@ export type ProcessLockOwner = { */ export type ProcessLockOwnerRecord = ProcessLockOwner & { claimToken: string | null; + /** + * Which loading of this module issued the claim. A pid names a process, not a copy of this file: + * two bundles of it in one process share the pid and the start time, and only one of them holds + * the other's tokens. Absent on a record written before claims carried an issuer. + */ + claimIssuerId?: string; }; /** Gives a lock back. Rejects when the lock is standing and this process cannot prove it owns it. */ @@ -72,6 +78,9 @@ type ProcessLockOwnerReading = */ const liveClaimTokens = new Set(); +/** Which loading of this module issues this process's claims. See `ProcessLockOwnerRecord`. */ +const CLAIM_ISSUER_ID = crypto.randomUUID(); + export async function acquireProcessLock(params: { lockDirPath: string; owner: ProcessLockOwner; @@ -89,7 +98,7 @@ export async function acquireProcessLock(params: { fs.mkdirSync(path.dirname(lockDirPath), { recursive: true }); const claimToken = crypto.randomUUID(); - const claim: ProcessLockOwnerRecord = { ...owner, claimToken }; + const claim: ProcessLockOwnerRecord = { ...owner, claimToken, claimIssuerId: CLAIM_ISSUER_ID }; while (Date.now() < deadline) { try { @@ -428,6 +437,7 @@ function parseProcessLockOwner(contents: string): ProcessLockOwnerRecord | null // A record written before claims were tokenized names a process without saying which // acquisition it was, which no release can match and no reclaim can be blamed for. claimToken: typeof record.claimToken === 'string' ? record.claimToken : null, + claimIssuerId: typeof record.claimIssuerId === 'string' ? record.claimIssuerId : undefined, }; } @@ -473,7 +483,10 @@ function isLiveProcessLockOwner(owner: ProcessLockOwner): boolean { * for the pid would be waiting for this process to restart. */ function isSpentOwnClaim(owner: ProcessLockOwnerRecord): boolean { - return ( - owner.pid === process.pid && owner.claimToken !== null && !liveClaimTokens.has(owner.claimToken) - ); + if (owner.pid !== process.pid || owner.claimToken === null) return false; + // A token this loading of the module never issued is either a claim by another loading in the + // same process, which is live and not ours to judge, or a record from before issuers existed, + // which is no evidence of a spent claim either. Both stay subject to the liveness answer. + if (owner.claimIssuerId !== CLAIM_ISSUER_ID) return false; + return !liveClaimTokens.has(owner.claimToken); } From 91453f950310682560b5eec22a9f1067d1ccd1d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 15:50:24 +0200 Subject: [PATCH 10/18] fix(apple-runner): one ordered give-back decides what the caller hears MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `release()` reconciled the host's device set inside a `try` and handed the lock back in a `finally`, so when both failed — and they fail together, being two writes to the same directory — the `finally` threw `ownerReleaseUnverified` over the restore's EACCES, and the best-effort door then dropped it as the one failure it is allowed to drop. A `~/Library/Developer/XCTestDevices` left pointing at this simulator's set is a fact about the machine that outlives the request, and the only report named the lock instead. The strict door was wrong too, in the other direction: it reported a lock problem for a restore problem. There is one give-back now, and it runs in one order: restore, then release. The restore failure always outranks the release, and the lock goes back regardless so the next acquire does not wait on a claim nobody is acting on. Whatever the release could not do is recorded either way, with the unverified release left to the log at the lock that already keeps it, so the two doors differ in exactly one thing: whether the caller also throws that failure. The same rule reaches the two paths that have no handle to give back. The simulator whose set already is `XCTestDevices` used to run its release inside the `try`, where an unverified ownership check fell into the catch, reconciled, released again, and raised "Failed to redirect XCTest device set path" for a redirect that was never needed; it now carries on and the lock's own log line stands. And a redirect that never got installed reports the redirect even when the clean-up it runs on the way out fails too, instead of the clean-up taking the report. The precedence rule stays in the handle rather than moving into `withProcessLock`: that helper knows about locks, and which of "the host's device set could not be restored" and "the claim could not be verified" a caller should hear is a fact about this redirect, not about locking. The rule was checked where it bites. Both faults at once — `renameSync` refusing the restore and the record refusing the unlink — now reach either door as EACCES, with the lock still standing as proof the pair really happened; reverting the order to the old `finally` turns that test red through both doors. A no-redirect simulator with an unverifiable release answers a null handle, and putting the release back inside the `try` brings the misleading message back verbatim. A redirect whose install and clean-up both fail still says "Failed to redirect XCTest device set path" with the install's EPERM inside it. The redirect double's doc comment, which had lost its way between two edits, says what it now means, and the CHANGELOG describes the `withProcessLock` migration this PR carries rather than only the lock fix that started it. --- CHANGELOG.md | 15 +++ .../runner-device-set-give-back.test.ts | 71 ++++++++++++ .../__tests__/runner-device-set.test.ts | 100 +++++++++++++++++ .../__tests__/runner-session-fixtures.ts | 5 +- .../src/runner/runner-device-set.ts | 104 ++++++++++++------ 5 files changed, 262 insertions(+), 33 deletions(-) create mode 100644 packages/platform-apple/src/runner/__tests__/runner-device-set-give-back.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 662cb88e4d..fe6bcd93bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -153,6 +153,21 @@ after 30 s on every runner build or launch until the process restarted. The claim inside is spent the moment the release is asked for, so a reclaim here now reads it as dead and takes the path back, and the failed release is recorded in the request log as `process_lock_release_unverified`. + A record is only read that way when this loading of the lock code issued it: a second bundled copy + of the module in the same process shares the pid and cannot have a live claim cleared under it. +- Changed (lock errors): code that takes a process lock now answers one question in one helper, which + of its two failures the caller hears. The work inside the lock outranks a lock that could not be + handed back, so a build or a publish that failed keeps its own error instead of being replaced by + `Timed out waiting for …`, and work that succeeded still reports the lock it could not give back. + The Apple runner's artifact, cache, lease and disposal paths, the managed-allocation store, the + device-claim store, atomic file publishes, the Swift recording cache and the agent-browser setup + moved onto it, replacing hand-written try/catch pairs that each chose differently. +- Fixed: the redirect of `~/Library/Developer/XCTestDevices` gives itself back in one order — restore + the host's own device set, then release the lock — and a restore that was refused is what the caller + is told, whichever give-back door a teardown used. The lock's own complaint used to replace it in a + `finally` and the best-effort door then dropped it, leaving the symlink pointed at the agent-device + simulator set with nothing said about why. A simulator whose set already is `XCTestDevices` is no + longer failed by a lock it could not verify, either. - Changed (sessions): the implicit session is now keyed by workspace **and platform**, so one checkout can drive iOS and Android without inventing a `--session` name for every command (#2580). An diff --git a/packages/platform-apple/src/runner/__tests__/runner-device-set-give-back.test.ts b/packages/platform-apple/src/runner/__tests__/runner-device-set-give-back.test.ts new file mode 100644 index 0000000000..c403e80055 --- /dev/null +++ b/packages/platform-apple/src/runner/__tests__/runner-device-set-give-back.test.ts @@ -0,0 +1,71 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { afterEach, 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'; + +// Which failure a redirect hands the caller is decided once, in the give-back. These tests reach the +// paths that have no handle to give back — the simulator that needs no redirect, and the redirect +// that never got installed — by answering the lock at the seam the module already acquires it +// through, which is the only window where a release can be made to fail from inside an acquire. +const { lockSeam } = vi.hoisted(() => ({ + lockSeam: { + override: null as null | (() => Promise<() => Promise>), + }, +})); + +vi.mock('../host.ts', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + acquireProcessLock: async (params: Parameters[0]) => + lockSeam.override ? await lockSeam.override() : await actual.acquireProcessLock(params), + }; +}); + +import { acquireXcodebuildSimulatorSetRedirect } from '../runner-device-set.ts'; + +function unverifiedRelease(): () => Promise { + return async () => { + throw new AppError('COMMAND_FAILED', 'Could not verify ownership of XCTest device set lock', { + ownerReleaseUnverified: true, + }); + }; +} + +afterEach(() => { + lockSeam.override = null; +}); + +test('a simulator already on the host device set is not failed by a lock it could not verify', async () => { + const root = mkdtempForTestSync('device-set-no-redirect-'); + try { + const xctestDeviceSetPath = path.join(root, 'Library', 'Developer', 'XCTestDevices'); + fs.mkdirSync(xctestDeviceSetPath, { recursive: true }); + // This simulator's set and the host's `XCTestDevices` are the same directory, so nothing is + // redirected and the caller is told so with a null handle. Before the give-back rule, the + // release inside the try fell into the catch, which reconciled, released again, and raised + // "Failed to redirect XCTest device set path" for a redirect that was never needed. + lockSeam.override = async () => unverifiedRelease(); + const device: DeviceInfo = { + platform: 'apple', + id: 'sim-host-set', + name: 'iPhone Simulator', + kind: 'simulator', + appleOs: 'ios', + booted: true, + simulatorSetPath: xctestDeviceSetPath, + }; + + const redirect = await acquireXcodebuildSimulatorSetRedirect(device, { + lockDirPath: path.join(root, '.agent-device', 'xctest-device-set.lock'), + xctestDeviceSetPath, + }); + + assert.equal(redirect, null); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } +}); diff --git a/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts index 48c2c2d448..96bfabe8d7 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts @@ -171,3 +171,103 @@ test('a redirect that could not restore the host device set reports it instead o } }); }); + +test('a restore that was refused outranks the lock that could not be verified', async () => { + // Both steps lean on the same filesystem, so they fail together. The `finally` that handed the lock + // back used to replace the restore's EACCES with `ownerReleaseUnverified`, which the best-effort + // door then dropped and the strict door then reported in its place: the host kept an + // `XCTestDevices` pointing at this simulator's set, and the only report named the lock. + for (const door of ['release', 'releaseBestEffort'] as const) { + await withTempDir(`device-set-both-fail-${door}-`, async (root) => { + const paths = makeRedirectPaths(root); + fs.mkdirSync(paths.requestedSetPath, { recursive: true }); + 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); + makeReleaseUnverifiable(paths); + + 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, to); + }) as typeof fs.renameSync); + + try { + await assert.rejects( + () => redirect![door](), + (error: unknown) => (error as NodeJS.ErrnoException).code === 'EACCES', + door, + ); + assert.equal(attempted, true, door); + // The lock went back into the same refusing filesystem, which is what makes this the pair. + assert.equal(fs.existsSync(paths.lockDirPath), true, `${door}: the release failed too`); + } finally { + renameSpy.mockRestore(); + } + }); + } +}); + +test('a redirect that could not be installed reports the redirect, not the failed clean-up', async () => { + await withTempDir('device-set-install-failure-', async (root) => { + const paths = makeRedirectPaths(root); + fs.mkdirSync(paths.requestedSetPath, { recursive: true }); + fs.mkdirSync(paths.xctestDeviceSetPath, { recursive: true }); + fs.writeFileSync(path.join(paths.xctestDeviceSetPath, 'host-device.txt'), 'the host owns this'); + + // The install breaks after the host's set is renamed into the backup, and the restore that the + // catch runs to undo it breaks too. Two failures, one report: the redirect that did not happen. + let restoreAttempted = false; + const realRename = fs.renameSync; + const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation((( + from: fs.PathLike, + to: fs.PathLike, + ) => { + if (String(to) === paths.xctestDeviceSetPath) { + restoreAttempted = true; + throw Object.assign(new Error('EACCES: permission denied'), { code: 'EACCES' }); + } + return realRename(from, to); + }) as typeof fs.renameSync); + const symlinkSpy = vi.spyOn(fs, 'symlinkSync').mockImplementation((() => { + throw Object.assign(new Error('EPERM: operation not permitted'), { code: 'EPERM' }); + }) as typeof fs.symlinkSync); + + try { + await assert.rejects( + () => + acquireXcodebuildSimulatorSetRedirect(makeScopedSimulator(paths), { + lockDirPath: paths.lockDirPath, + xctestDeviceSetPath: paths.xctestDeviceSetPath, + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.message, 'Failed to redirect XCTest device set path'); + assert.match(String(error.details?.error), /EPERM/); + return true; + }, + ); + assert.equal(restoreAttempted, true); + // The clean-up did not run to completion, and the lock went back regardless: the caller is + // free to try again rather than wait 30 s on a claim nobody holds. + assert.equal(fs.existsSync(paths.lockDirPath), false); + } finally { + renameSpy.mockRestore(); + symlinkSpy.mockRestore(); + } + }); +}); diff --git a/packages/platform-apple/src/runner/__tests__/runner-session-fixtures.ts b/packages/platform-apple/src/runner/__tests__/runner-session-fixtures.ts index 2c7bc07bf6..6aef63ff4c 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-session-fixtures.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-session-fixtures.ts @@ -186,8 +186,9 @@ export function makeClassifyOwnerLivenessViaMocks(deps: { } /** - * The give-back a launched session holds. One spy answers both strictnesses: the the host's device set came back, and which door it came back - * through is pinned where the handle is made, in `runner-device-set.test.ts`. + * The give-back a launched session holds. One spy answers both doors, because these tests ask whether + * the host's device set came back; which door it came back through is pinned where the handle is + * made, in `runner-device-set.test.ts`. */ export const redirectRelease = vi.fn(async () => {}); diff --git a/packages/platform-apple/src/runner/runner-device-set.ts b/packages/platform-apple/src/runner/runner-device-set.ts index f93000ab61..3669469960 100644 --- a/packages/platform-apple/src/runner/runner-device-set.ts +++ b/packages/platform-apple/src/runner/runner-device-set.ts @@ -20,13 +20,16 @@ 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. */ + /** + * Runs the ordered give-back — restore the host's device set, then release the lock — and reports + * both failures, throwing the restore failure when there is one and the release failure otherwise. + */ release: () => Promise; /** - * 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. + * The same ordered give-back for a caller whose own outcome is already decided — a launch that + * failed, a teardown that ran. A release that cannot verify ownership is dropped rather than + * thrown: that claim is spent, and the next reclaim from this process reads it as dead. A failure + * to restore the host's own `XCTestDevices` outranks it and is thrown all the same. */ releaseBestEffort: () => Promise; }; @@ -101,13 +104,25 @@ export async function acquireXcodebuildSimulatorSetRedirect( }, }); + const giveLockBack = async (): Promise => { + try { + await releaseLock(); + return null; + } catch (error) { + return error; + } + }; + try { reconcileXcodebuildSimulatorSetRedirect({ xctestDeviceSetPath, backupPath, }); if (sameResolvedPath(requestedSetPath, xctestDeviceSetPath)) { - await releaseLock(); + // Nothing was redirected, so nothing was displaced and this simulator needs no handle: the + // caller has no redirect to report, and a lock it never held must not arrive as one. The + // give-back records its own failure where the claim lives. + recordHandBackFailure(await giveLockBack(), lockDirPath); return null; } @@ -120,13 +135,21 @@ export async function acquireXcodebuildSimulatorSetRedirect( xctestDeviceSetPath, }); } catch (error) { - reconcileXcodebuildSimulatorSetRedirect({ - xctestDeviceSetPath, - backupPath, - }); - // The redirect failure is the reportable fact; an unverified release leaves the lock - // to the stale-clear path rather than displacing it. - await releaseLock().catch(() => undefined); + // The redirect failure outranks everything the hand-back does, including a restore that could not + // run on the way out. Both are recorded; the caller hears why the redirect failed. + try { + reconcileXcodebuildSimulatorSetRedirect({ + xctestDeviceSetPath, + backupPath, + }); + } catch (restoreError) { + emitDiagnostic({ + level: 'warn', + phase: 'ios_runner_xctest_device_set_restore_failed', + data: { xctestDeviceSetPath, backupPath, error: String(restoreError) }, + }); + } + recordHandBackFailure(await giveLockBack(), lockDirPath); throw new AppError('COMMAND_FAILED', 'Failed to redirect XCTest device set path', { requestedSetPath, xctestDeviceSetPath, @@ -135,37 +158,56 @@ export async function acquireXcodebuildSimulatorSetRedirect( }); } - let released = false; - const release = async () => { - if (released) { + let givenBack = false; + // One ordered give-back: restore the host's own device set, then hand the lock back. A restore that + // could not run is a fact about this machine — the symlink stays pointed at this simulator's set and + // every later `simctl` run sees the wrong devices — so its failure always outranks the release, and + // the lock goes back anyway so the next acquire does not wait on it. Whatever the release could not + // do is recorded either way; the two doors differ only in whether the caller also throws it. + const giveBack = async (reportUnverifiedRelease: boolean): Promise => { + if (givenBack) { return; } - released = true; + givenBack = true; + let restoreFailure: unknown = null; try { reconcileXcodebuildSimulatorSetRedirect({ xctestDeviceSetPath, backupPath, }); - } finally { - await releaseLock(); + } catch (error) { + restoreFailure = error; + } + const releaseFailure = await giveLockBack(); + recordHandBackFailure(releaseFailure, lockDirPath); + if (restoreFailure !== null) { + throw restoreFailure; + } + if (releaseFailure !== null && reportUnverifiedRelease) { + throw releaseFailure; } }; return { - release, - releaseBestEffort: async () => { - try { - 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; - } - }, + release: () => giveBack(true), + releaseBestEffort: () => giveBack(false), }; } +/** + * Records a hand-back failure that no caller is being made to throw. A release whose ownership could + * not be verified is already recorded where the claim lives, so only the rest reaches the log. + */ +function recordHandBackFailure(error: unknown, lockDirPath: string): void { + if (error === null || isOwnerReleaseUnverified(error)) { + return; + } + emitDiagnostic({ + level: 'warn', + phase: 'ios_runner_xctest_device_set_hand_back_failed', + data: { lockDirPath, error: String(error) }, + }); +} + function isOwnerReleaseUnverified(error: unknown): boolean { return error instanceof AppError && error.details?.ownerReleaseUnverified === true; } From db0ab80cee8d7ca811f9cdbd46fe4649a182588f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 17:30:57 +0200 Subject: [PATCH 11/18] refactor(runner): one hand-back answers for all three redirect exits The install-failure catch kept its own try/catch around the restore, so a restore that could not run reached only the log while the caller saw the redirect failure alone. It now uses the same ordered hand-back as the handle and the no-redirect exit, and carries the restore failure in the error's details with a hint naming the path the host's device set is waiting at. --- CHANGELOG.md | 3 +- .../__tests__/runner-device-set.test.ts | 52 +++++++- .../src/runner/runner-device-set.ts | 122 +++++++++--------- 3 files changed, 117 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe6bcd93bd..a9779ec0f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -167,7 +167,8 @@ is told, whichever give-back door a teardown used. The lock's own complaint used to replace it in a `finally` and the best-effort door then dropped it, leaving the symlink pointed at the agent-device simulator set with nothing said about why. A simulator whose set already is `XCTestDevices` is no - longer failed by a lock it could not verify, either. + longer failed by a lock it could not verify, either, and a redirect that could not be installed names + the path a failed restore left the host's device set at instead of only logging it. - Changed (sessions): the implicit session is now keyed by workspace **and platform**, so one checkout can drive iOS and Android without inventing a `--session` name for every command (#2580). An diff --git a/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts index 96bfabe8d7..6c9547acc2 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts @@ -26,6 +26,7 @@ const iosSimulator: DeviceInfo = { type RedirectPaths = { requestedSetPath: string; xctestDeviceSetPath: string; + backupPath: string; lockDirPath: string; }; @@ -33,6 +34,7 @@ function makeRedirectPaths(root: string): RedirectPaths { return { requestedSetPath: path.join(root, 'requested'), xctestDeviceSetPath: path.join(root, 'Library', 'Developer', 'XCTestDevices'), + backupPath: path.join(root, 'Library', 'Developer', 'XCTestDevices.set-aside'), lockDirPath: path.join(root, '.agent-device', 'xctest-device-set.lock'), }; } @@ -222,6 +224,47 @@ test('a restore that was refused outranks the lock that could not be verified', } }); +test('the host’s device set is back before the lock is', async () => { + await withTempDir('device-set-hand-back-order-', async (root) => { + const paths = makeRedirectPaths(root); + fs.mkdirSync(paths.requestedSetPath, { recursive: true }); + fs.mkdirSync(paths.xctestDeviceSetPath, { recursive: true }); + + // The lock is what lets another runner build read the host's `XCTestDevices`, so the order the + // give-back works in is the contract: releasing first would hand out a directory that is still a + // symlink into this simulator's set. The events record when each step actually happened. + const events: string[] = []; + const realRename = fs.renameSync; + const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation(((from, to) => { + const source = String(from); + const target = String(to); + if (target === paths.backupPath) events.push('renamed-aside'); + else if (source === paths.backupPath) events.push('restored'); + else if (target === paths.xctestDeviceSetPath) events.push('symlink-installed'); + return realRename(from, to); + }) as typeof fs.renameSync); + const realRemoveDir = fs.rmdirSync; + const removeDirSpy = vi.spyOn(fs, 'rmdirSync').mockImplementation(((target, ...rest) => { + if (String(target) === paths.lockDirPath) events.push('lock-released'); + return realRemoveDir(target, ...rest); + }) as typeof fs.rmdirSync); + + try { + const handle = await acquireXcodebuildSimulatorSetRedirect(makeScopedSimulator(paths), { + ...redirectOptions(paths), + backupPath: paths.backupPath, + }); + assert.ok(handle); + await handle.release(); + + assert.deepEqual(events, ['renamed-aside', 'symlink-installed', 'restored', 'lock-released']); + } finally { + renameSpy.mockRestore(); + removeDirSpy.mockRestore(); + } + }); +}); + test('a redirect that could not be installed reports the redirect, not the failed clean-up', async () => { await withTempDir('device-set-install-failure-', async (root) => { const paths = makeRedirectPaths(root); @@ -230,7 +273,8 @@ test('a redirect that could not be installed reports the redirect, not the faile fs.writeFileSync(path.join(paths.xctestDeviceSetPath, 'host-device.txt'), 'the host owns this'); // The install breaks after the host's set is renamed into the backup, and the restore that the - // catch runs to undo it breaks too. Two failures, one report: the redirect that did not happen. + // catch runs to undo it breaks too. Two failures, one report: the redirect that did not happen, + // carrying the restore that could not run and naming where the host's device set is waiting. let restoreAttempted = false; const realRename = fs.renameSync; const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation((( @@ -253,11 +297,17 @@ test('a redirect that could not be installed reports the redirect, not the faile acquireXcodebuildSimulatorSetRedirect(makeScopedSimulator(paths), { lockDirPath: paths.lockDirPath, xctestDeviceSetPath: paths.xctestDeviceSetPath, + backupPath: paths.backupPath, }), (error: unknown) => { assert.ok(error instanceof AppError); assert.equal(error.message, 'Failed to redirect XCTest device set path'); assert.match(String(error.details?.error), /EPERM/); + assert.match(String(error.details?.restoreError), /EACCES/); + assert.ok( + String(error.details?.hint).includes(paths.backupPath), + `the hint must name where the host's device set is: ${String(error.details?.hint)}`, + ); return true; }, ); diff --git a/packages/platform-apple/src/runner/runner-device-set.ts b/packages/platform-apple/src/runner/runner-device-set.ts index 3669469960..5343c788cc 100644 --- a/packages/platform-apple/src/runner/runner-device-set.ts +++ b/packages/platform-apple/src/runner/runner-device-set.ts @@ -104,28 +104,19 @@ export async function acquireXcodebuildSimulatorSetRedirect( }, }); - const giveLockBack = async (): Promise => { - try { - await releaseLock(); - return null; - } catch (error) { - return error; - } - }; + const paths = { xctestDeviceSetPath, backupPath }; - try { - reconcileXcodebuildSimulatorSetRedirect({ - xctestDeviceSetPath, - backupPath, - }); - if (sameResolvedPath(requestedSetPath, xctestDeviceSetPath)) { - // Nothing was redirected, so nothing was displaced and this simulator needs no handle: the - // caller has no redirect to report, and a lock it never held must not arrive as one. The - // give-back records its own failure where the claim lives. - recordHandBackFailure(await giveLockBack(), lockDirPath); - return null; - } + if (sameResolvedPath(requestedSetPath, xctestDeviceSetPath)) { + // Nothing is displaced and the caller gets no handle, so this hand-back has no failure of its own + // to report: a lock this simulator never needed must not arrive as a redirect problem. Whatever it + // could not verify is recorded where the claim lives. + recordReleaseFailure((await handBackDeviceSet(paths, releaseLock)).releaseFailure, lockDirPath); + return null; + } + try { + // Clean up whatever an earlier run left, out of the way of the rename below. + reconcileXcodebuildSimulatorSetRedirect(paths); fs.mkdirSync(requestedSetPath, { recursive: true }); if (fs.existsSync(xctestDeviceSetPath)) { fs.renameSync(xctestDeviceSetPath, backupPath); @@ -135,56 +126,37 @@ export async function acquireXcodebuildSimulatorSetRedirect( xctestDeviceSetPath, }); } catch (error) { - // The redirect failure outranks everything the hand-back does, including a restore that could not - // run on the way out. Both are recorded; the caller hears why the redirect failed. - try { - reconcileXcodebuildSimulatorSetRedirect({ - xctestDeviceSetPath, - backupPath, - }); - } catch (restoreError) { - emitDiagnostic({ - level: 'warn', - phase: 'ios_runner_xctest_device_set_restore_failed', - data: { xctestDeviceSetPath, backupPath, error: String(restoreError) }, - }); - } - recordHandBackFailure(await giveLockBack(), lockDirPath); + const handBack = await handBackDeviceSet(paths, releaseLock); + recordReleaseFailure(handBack.releaseFailure, lockDirPath); throw new AppError('COMMAND_FAILED', 'Failed to redirect XCTest device set path', { requestedSetPath, xctestDeviceSetPath, backupPath, error: String(error), + ...(handBack.restoreFailure === null + ? {} + : { + restoreError: String(handBack.restoreFailure), + hint: + `The host's own device set is still renamed aside at ${backupPath}: restore it, ` + + 'or remove that path, before another runner build redirects it.', + }), }); } let givenBack = false; - // One ordered give-back: restore the host's own device set, then hand the lock back. A restore that - // could not run is a fact about this machine — the symlink stays pointed at this simulator's set and - // every later `simctl` run sees the wrong devices — so its failure always outranks the release, and - // the lock goes back anyway so the next acquire does not wait on it. Whatever the release could not - // do is recorded either way; the two doors differ only in whether the caller also throws it. const giveBack = async (reportUnverifiedRelease: boolean): Promise => { if (givenBack) { return; } givenBack = true; - let restoreFailure: unknown = null; - try { - reconcileXcodebuildSimulatorSetRedirect({ - xctestDeviceSetPath, - backupPath, - }); - } catch (error) { - restoreFailure = error; + const handBack = await handBackDeviceSet(paths, releaseLock); + recordReleaseFailure(handBack.releaseFailure, lockDirPath); + if (handBack.restoreFailure !== null) { + throw handBack.restoreFailure; } - const releaseFailure = await giveLockBack(); - recordHandBackFailure(releaseFailure, lockDirPath); - if (restoreFailure !== null) { - throw restoreFailure; - } - if (releaseFailure !== null && reportUnverifiedRelease) { - throw releaseFailure; + if (handBack.releaseFailure !== null && reportUnverifiedRelease) { + throw handBack.releaseFailure; } }; return { @@ -193,11 +165,45 @@ export async function acquireXcodebuildSimulatorSetRedirect( }; } +type DeviceSetHandBack = { + /** The host's own `XCTestDevices` could not be put back, so the symlink is still in its place. */ + restoreFailure: unknown; + /** The lock could not be given back, or could not be verified as ours when it was. */ + releaseFailure: unknown; +}; + +/** + * The one ordered hand-back, used by every path that leaves this redirect behind: restore the host's + * own device set, then release the lock. Neither step can hide the other, because a restore that could + * not run is a fact about this machine that outlives the request — every later `simctl` run sees the + * wrong devices — and the lock going back is what keeps the next acquire from waiting on a claim nobody + * is acting on. Deciding which of the two the caller hears is left to the caller that has a report to + * make: the redirect failure, the restore failure, or neither. + */ +async function handBackDeviceSet( + paths: { xctestDeviceSetPath: string; backupPath: string }, + releaseLock: () => Promise, +): Promise { + let restoreFailure: unknown = null; + let releaseFailure: unknown = null; + try { + reconcileXcodebuildSimulatorSetRedirect(paths); + } catch (error) { + restoreFailure = error; + } + try { + await releaseLock(); + } catch (error) { + releaseFailure = error; + } + return { restoreFailure, releaseFailure }; +} + /** - * Records a hand-back failure that no caller is being made to throw. A release whose ownership could - * not be verified is already recorded where the claim lives, so only the rest reaches the log. + * Records a lock release that no caller is being made to throw. A release whose ownership could not be + * verified is already recorded where the claim lives, so only the rest reaches the log. */ -function recordHandBackFailure(error: unknown, lockDirPath: string): void { +function recordReleaseFailure(error: unknown, lockDirPath: string): void { if (error === null || isOwnerReleaseUnverified(error)) { return; } From 530d818e0b879cbe8dc8957a4a3675734a9ff30e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 18:53:47 +0200 Subject: [PATCH 12/18] fix(apple-runner): a leftover redirect is undone before the redirect is judged The same-set check follows symlinks, and an interrupted build leaves XCTestDevices symlinked into a simulator's requested set. Read as "already the same set" it made this run hand the symlink back and quit, so the next xcodebuild built against the host's own devices. Reconcile runs first again, and a restore that cannot run stops the redirect with its own error instead of looking like a simulator that needs nothing. The hand-back records the release itself and reports the backup it actually found, so no exit reads one half of the result and misses the other. --- CHANGELOG.md | 7 +- .../__tests__/runner-device-set.test.ts | 123 ++++++++++++++++++ .../src/runner/runner-device-set.ts | 88 ++++++++++--- 3 files changed, 196 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a9779ec0f3..eebc1de872 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -167,8 +167,11 @@ is told, whichever give-back door a teardown used. The lock's own complaint used to replace it in a `finally` and the best-effort door then dropped it, leaving the symlink pointed at the agent-device simulator set with nothing said about why. A simulator whose set already is `XCTestDevices` is no - longer failed by a lock it could not verify, either, and a redirect that could not be installed names - the path a failed restore left the host's device set at instead of only logging it. + longer failed by a lock it could not verify, either. A leftover from an interrupted build — the host's + set renamed aside and `XCTestDevices` symlinked into a simulator's own set — is now put back before the + redirect decides whether it is needed, so the first build after an interruption still gets its own + device set instead of the host's. A redirect that could not be installed reports the restore that failed + with it, and names a backup path only when that backup is really on disk. - Changed (sessions): the implicit session is now keyed by workspace **and platform**, so one checkout can drive iOS and Android without inventing a `--session` name for every command (#2580). An diff --git a/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts index 6c9547acc2..9df0d7c45a 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts @@ -224,6 +224,85 @@ test('a restore that was refused outranks the lock that could not be verified', } }); +test('an interrupted build that left the symlink is redirected again, not read as already done', async () => { + await withTempDir('device-set-leftover-', async (root) => { + const paths = makeRedirectPaths(root); + fs.mkdirSync(paths.requestedSetPath, { recursive: true }); + // What an interrupted build leaves: the host's set renamed aside, and `XCTestDevices` a symlink + // pointing into this simulator's requested set. Following that symlink makes the two paths look + // identical, and deciding from that would hand the symlink back and let the next `xcodebuild` run + // against the host's own devices. + fs.mkdirSync(paths.backupPath, { recursive: true }); + fs.writeFileSync(path.join(paths.backupPath, 'host-device.txt'), 'the host owns this'); + fs.symlinkSync(paths.requestedSetPath, paths.xctestDeviceSetPath, 'dir'); + + const handle = await acquireXcodebuildSimulatorSetRedirect(makeScopedSimulator(paths), { + ...redirectOptions(paths), + backupPath: paths.backupPath, + }); + + try { + assert.ok(handle, 'this simulator needs its own redirect, leftovers and all'); + assert.equal( + fs.realpathSync.native(paths.xctestDeviceSetPath), + fs.realpathSync.native(paths.requestedSetPath), + ); + // The leftover backup was put back where it belongs and then renamed aside by this run, so the + // host's device set is whole exactly once. + assert.equal( + fs.readFileSync(path.join(paths.backupPath, 'host-device.txt'), 'utf8'), + 'the host owns this', + ); + + await handle.release(); + assert.equal( + fs.readFileSync(path.join(paths.xctestDeviceSetPath, 'host-device.txt'), 'utf8'), + 'the host owns this', + ); + } finally { + await handle?.releaseBestEffort(); + } + }); +}); + +test('a restore that was refused on the way in is not reported as a simulator that needs no redirect', async () => { + await withTempDir('device-set-leftover-restore-failed-', async (root) => { + const paths = makeRedirectPaths(root); + fs.mkdirSync(paths.requestedSetPath, { recursive: true }); + fs.mkdirSync(paths.backupPath, { recursive: true }); + fs.writeFileSync(path.join(paths.backupPath, 'host-device.txt'), 'the host owns this'); + fs.symlinkSync(paths.requestedSetPath, paths.xctestDeviceSetPath, 'dir'); + + // The same leftovers, with the rename that would put the host's set back refusing. Both paths still + // resolve to the same directory, so this is the moment where "nothing to do" and "the host has no + // device set" look identical from here. + const realRename = fs.renameSync; + const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation(((from, to) => { + if (String(to) === paths.xctestDeviceSetPath) { + throw Object.assign(new Error('EACCES: permission denied'), { code: 'EACCES' }); + } + return realRename(from, to); + }) as typeof fs.renameSync); + + try { + await assert.rejects( + () => + acquireXcodebuildSimulatorSetRedirect(makeScopedSimulator(paths), { + ...redirectOptions(paths), + backupPath: paths.backupPath, + }), + /EACCES/, + ); + // The host's set is still renamed aside, which is the fact the caller needs, and the lock went + // back anyway so the next acquire does not wait on this one. + assert.equal(fs.existsSync(paths.backupPath), true); + assert.equal(fs.existsSync(paths.lockDirPath), false); + } finally { + renameSpy.mockRestore(); + } + }); +}); + test('the host’s device set is back before the lock is', async () => { await withTempDir('device-set-hand-back-order-', async (root) => { const paths = makeRedirectPaths(root); @@ -265,6 +344,50 @@ test('the host’s device set is back before the lock is', async () => { }); }); +test('a redirect that failed before it moved anything names no backup that is not there', async () => { + await withTempDir('device-set-failed-before-rename-', async (root) => { + const paths = makeRedirectPaths(root); + fs.mkdirSync(paths.requestedSetPath, { recursive: true }); + fs.mkdirSync(paths.xctestDeviceSetPath, { recursive: true }); + fs.writeFileSync(path.join(paths.xctestDeviceSetPath, 'host-device.txt'), 'the host owns this'); + + // The install dies on the rename that was meant to move the host's set aside, so nothing ever left + // its place. A report that still said "still renamed aside at " would send the reader to a + // path that does not exist. + const realRename = fs.renameSync; + const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation(((from, to) => { + if (String(to) === paths.backupPath) { + throw Object.assign(new Error('EACCES: permission denied'), { code: 'EACCES' }); + } + return realRename(from, to); + }) as typeof fs.renameSync); + + try { + await assert.rejects( + () => + acquireXcodebuildSimulatorSetRedirect(makeScopedSimulator(paths), { + ...redirectOptions(paths), + backupPath: paths.backupPath, + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.message, 'Failed to redirect XCTest device set path'); + assert.match(String(error.details?.error), /EACCES/); + assert.equal(error.details?.restoreError, undefined); + assert.equal(error.details?.hint, undefined); + return true; + }, + ); + assert.equal( + fs.readFileSync(path.join(paths.xctestDeviceSetPath, 'host-device.txt'), 'utf8'), + 'the host owns this', + ); + } finally { + renameSpy.mockRestore(); + } + }); +}); + test('a redirect that could not be installed reports the redirect, not the failed clean-up', async () => { await withTempDir('device-set-install-failure-', async (root) => { const paths = makeRedirectPaths(root); diff --git a/packages/platform-apple/src/runner/runner-device-set.ts b/packages/platform-apple/src/runner/runner-device-set.ts index 5343c788cc..9cac52aece 100644 --- a/packages/platform-apple/src/runner/runner-device-set.ts +++ b/packages/platform-apple/src/runner/runner-device-set.ts @@ -106,17 +106,25 @@ export async function acquireXcodebuildSimulatorSetRedirect( const paths = { xctestDeviceSetPath, backupPath }; + // Undo what an earlier build left before anything here decides about this simulator. The same-set + // check below follows symlinks, and an interrupted build leaves `XCTestDevices` symlinked into a + // simulator's requested set: read that as "already the same set" and this run would hand the symlink + // back and the next `xcodebuild` would build against the host's own devices. A restore that could not + // run leaves that check and the rename below equally meaningless, so it stops the redirect as well. + try { + reconcileXcodebuildSimulatorSetRedirect(paths); + } catch { + await handBackOrRaise(paths, lockDirPath, releaseLock); + } + if (sameResolvedPath(requestedSetPath, xctestDeviceSetPath)) { - // Nothing is displaced and the caller gets no handle, so this hand-back has no failure of its own - // to report: a lock this simulator never needed must not arrive as a redirect problem. Whatever it - // could not verify is recorded where the claim lives. - recordReleaseFailure((await handBackDeviceSet(paths, releaseLock)).releaseFailure, lockDirPath); + // Nothing is displaced and the caller gets no handle: a lock this simulator never needed must not + // arrive as a redirect problem, and a host device set that could not be put back still must. + await handBackOrRaise(paths, lockDirPath, releaseLock); return null; } try { - // Clean up whatever an earlier run left, out of the way of the rename below. - reconcileXcodebuildSimulatorSetRedirect(paths); fs.mkdirSync(requestedSetPath, { recursive: true }); if (fs.existsSync(xctestDeviceSetPath)) { fs.renameSync(xctestDeviceSetPath, backupPath); @@ -126,8 +134,7 @@ export async function acquireXcodebuildSimulatorSetRedirect( xctestDeviceSetPath, }); } catch (error) { - const handBack = await handBackDeviceSet(paths, releaseLock); - recordReleaseFailure(handBack.releaseFailure, lockDirPath); + const handBack = await handBackDeviceSet(paths, lockDirPath, releaseLock); throw new AppError('COMMAND_FAILED', 'Failed to redirect XCTest device set path', { requestedSetPath, xctestDeviceSetPath, @@ -137,9 +144,13 @@ export async function acquireXcodebuildSimulatorSetRedirect( ? {} : { restoreError: String(handBack.restoreFailure), - hint: - `The host's own device set is still renamed aside at ${backupPath}: restore it, ` + - 'or remove that path, before another runner build redirects it.', + ...(handBack.renamedAsidePath === null + ? {} + : { + hint: + `The host's own device set is still renamed aside at ${handBack.renamedAsidePath}: ` + + 'restore it, or remove that path, before another runner build redirects it.', + }), }), }); } @@ -150,8 +161,7 @@ export async function acquireXcodebuildSimulatorSetRedirect( return; } givenBack = true; - const handBack = await handBackDeviceSet(paths, releaseLock); - recordReleaseFailure(handBack.releaseFailure, lockDirPath); + const handBack = await handBackDeviceSet(paths, lockDirPath, releaseLock); if (handBack.restoreFailure !== null) { throw handBack.restoreFailure; } @@ -165,38 +175,76 @@ export async function acquireXcodebuildSimulatorSetRedirect( }; } +/** What one ordered hand-back found, with neither failure able to hide the other. */ type DeviceSetHandBack = { - /** The host's own `XCTestDevices` could not be put back, so the symlink is still in its place. */ + /** The host's own `XCTestDevices` could not be put back, so a symlink or nothing is in its place. */ restoreFailure: unknown; + /** The path the host's own set is waiting at when a restore left it renamed aside, else null. */ + renamedAsidePath: string | null; /** The lock could not be given back, or could not be verified as ours when it was. */ releaseFailure: unknown; }; /** * The one ordered hand-back, used by every path that leaves this redirect behind: restore the host's - * own device set, then release the lock. Neither step can hide the other, because a restore that could - * not run is a fact about this machine that outlives the request — every later `simctl` run sees the - * wrong devices — and the lock going back is what keeps the next acquire from waiting on a claim nobody - * is acting on. Deciding which of the two the caller hears is left to the caller that has a report to - * make: the redirect failure, the restore failure, or neither. + * own device set, then release the lock, and record whatever the release could not do. Neither step can + * hide the other, because a restore that could not run is a fact about this machine that outlives the + * request — every later `simctl` run sees the wrong devices — and the lock going back is what keeps the + * next acquire from waiting on a claim nobody is acting on. Which of the two a caller *hears* stays with + * the caller: a redirect that failed reports both, and one that merely ended reports the restore. */ async function handBackDeviceSet( paths: { xctestDeviceSetPath: string; backupPath: string }, + lockDirPath: string, releaseLock: () => Promise, ): Promise { let restoreFailure: unknown = null; + let renamedAsidePath: string | null = null; let releaseFailure: unknown = null; try { + // Idempotent, so an exit that already reconciled on its way to this decision pays only a look. reconcileXcodebuildSimulatorSetRedirect(paths); } catch (error) { restoreFailure = error; + // Observed here, while the lock is still held and the release has not moved anything. + renamedAsidePath = findDeviceSetBackup(paths.backupPath); } try { await releaseLock(); } catch (error) { releaseFailure = error; } - return { restoreFailure, releaseFailure }; + recordReleaseFailure(releaseFailure, lockDirPath); + return { restoreFailure, renamedAsidePath, releaseFailure }; +} + +/** + * Hands the device set back for an exit that has no report of its own to make, and raises the restore + * when the host's own set is still not in place. The hand-back records the release, so there is nothing + * here that could read one half of its result and miss the other. + */ +async function handBackOrRaise( + paths: { xctestDeviceSetPath: string; backupPath: string }, + lockDirPath: string, + releaseLock: () => Promise, +): Promise { + const { restoreFailure } = await handBackDeviceSet(paths, lockDirPath, releaseLock); + if (restoreFailure !== null) { + throw restoreFailure; + } +} + +/** + * The backup that holds the host's own device set, when one is really on disk. The path this run would + * have written is not the only candidate: an older version renamed it beside a different name, and that + * leftover is just as much the host's device set. + */ +function findDeviceSetBackup(backupPath: string): string | null { + return ( + [backupPath, ...findLegacyXcodebuildSimulatorSetBackups(backupPath)].find((candidate) => + fs.existsSync(candidate), + ) ?? null + ); } /** From d081f84ded405e39209108fd19e0484d46d239e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 22:40:54 +0200 Subject: [PATCH 13/18] refactor(apple-runner): the redirect decides in place and reports through one builder --- .../src/runner/runner-device-set.ts | 74 ++++++++++++------- 1 file changed, 46 insertions(+), 28 deletions(-) diff --git a/packages/platform-apple/src/runner/runner-device-set.ts b/packages/platform-apple/src/runner/runner-device-set.ts index 9cac52aece..166e3d3cd0 100644 --- a/packages/platform-apple/src/runner/runner-device-set.ts +++ b/packages/platform-apple/src/runner/runner-device-set.ts @@ -125,34 +125,10 @@ export async function acquireXcodebuildSimulatorSetRedirect( } try { - fs.mkdirSync(requestedSetPath, { recursive: true }); - if (fs.existsSync(xctestDeviceSetPath)) { - fs.renameSync(xctestDeviceSetPath, backupPath); - } - installXcodebuildSimulatorSetSymlink({ - requestedSetPath, - xctestDeviceSetPath, - }); + installDeviceSetRedirect(paths, requestedSetPath); } catch (error) { const handBack = await handBackDeviceSet(paths, lockDirPath, releaseLock); - throw new AppError('COMMAND_FAILED', 'Failed to redirect XCTest device set path', { - requestedSetPath, - xctestDeviceSetPath, - backupPath, - error: String(error), - ...(handBack.restoreFailure === null - ? {} - : { - restoreError: String(handBack.restoreFailure), - ...(handBack.renamedAsidePath === null - ? {} - : { - hint: - `The host's own device set is still renamed aside at ${handBack.renamedAsidePath}: ` + - 'restore it, or remove that path, before another runner build redirects it.', - }), - }), - }); + throw redirectFailure(error, handBack, { requestedSetPath, ...paths }); } let givenBack = false; @@ -175,6 +151,48 @@ export async function acquireXcodebuildSimulatorSetRedirect( }; } +/** The two paths a redirect moves around: where the host keeps its set, and where this run put it. */ +type DeviceSetPaths = { + xctestDeviceSetPath: string; + backupPath: string; +}; + +/** The rename that gives this simulator the host's slot, and the symlink that occupies it. */ +function installDeviceSetRedirect(paths: DeviceSetPaths, requestedSetPath: string): void { + fs.mkdirSync(requestedSetPath, { recursive: true }); + if (fs.existsSync(paths.xctestDeviceSetPath)) { + fs.renameSync(paths.xctestDeviceSetPath, paths.backupPath); + } + installXcodebuildSimulatorSetSymlink({ + requestedSetPath, + xctestDeviceSetPath: paths.xctestDeviceSetPath, + }); +} + +/** + * Why this redirect did not happen, plus whatever the hand-back could not put right on the way out. A + * backup path is named only when that backup is really on disk: a reader sent to restore a path that + * does not exist learns the wrong lesson from this error. + */ +function redirectFailure( + cause: unknown, + handBack: DeviceSetHandBack, + paths: DeviceSetPaths & { requestedSetPath: string }, +): AppError { + return new AppError('COMMAND_FAILED', 'Failed to redirect XCTest device set path', { + ...paths, + error: String(cause), + ...(handBack.restoreFailure === null ? {} : { restoreError: String(handBack.restoreFailure) }), + ...(handBack.renamedAsidePath === null + ? {} + : { + hint: + `The host's own device set is still renamed aside at ${handBack.renamedAsidePath}: ` + + 'restore it, or remove that path, before another runner build redirects it.', + }), + }); +} + /** What one ordered hand-back found, with neither failure able to hide the other. */ type DeviceSetHandBack = { /** The host's own `XCTestDevices` could not be put back, so a symlink or nothing is in its place. */ @@ -194,7 +212,7 @@ type DeviceSetHandBack = { * the caller: a redirect that failed reports both, and one that merely ended reports the restore. */ async function handBackDeviceSet( - paths: { xctestDeviceSetPath: string; backupPath: string }, + paths: DeviceSetPaths, lockDirPath: string, releaseLock: () => Promise, ): Promise { @@ -224,7 +242,7 @@ async function handBackDeviceSet( * here that could read one half of its result and miss the other. */ async function handBackOrRaise( - paths: { xctestDeviceSetPath: string; backupPath: string }, + paths: DeviceSetPaths, lockDirPath: string, releaseLock: () => Promise, ): Promise { From e732fe641a15f365fa146c3c7806b09ab07bbe24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 16 Sep 2026 12:15:24 +0200 Subject: [PATCH 14/18] fix(platform-apple): exit the device-set acquire only by throw or return --- .../__tests__/runner-device-set.test.ts | 62 ++++++++++++++++- .../src/runner/runner-device-set.ts | 66 +++++++++---------- 2 files changed, 91 insertions(+), 37 deletions(-) diff --git a/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts index 9df0d7c45a..657cd41591 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts @@ -265,6 +265,59 @@ test('an interrupted build that left the symlink is redirected again, not read a }); }); +test('a restore that only works on the second look still ends the acquire', async () => { + await withTempDir('device-set-restore-retried-', async (root) => { + const paths = makeRedirectPaths(root); + fs.mkdirSync(paths.requestedSetPath, { recursive: true }); + fs.mkdirSync(paths.backupPath, { recursive: true }); + fs.writeFileSync(path.join(paths.backupPath, 'host-device.txt'), 'the host owns this'); + fs.symlinkSync(paths.requestedSetPath, paths.xctestDeviceSetPath, 'dir'); + + // A first rename that fails and a retry that would succeed. What the acquire must not do is decide + // from the state the failed restore left and then move the device set around without the lock. + let restoreAttempts = 0; + const realRename = fs.renameSync; + const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation(((from, to) => { + if (String(to) === paths.xctestDeviceSetPath) { + restoreAttempts += 1; + if (restoreAttempts === 1) { + throw Object.assign(new Error('EACCES: permission denied'), { code: 'EACCES' }); + } + } + return realRename(from, to); + }) as typeof fs.renameSync); + + try { + await assert.rejects( + () => + acquireXcodebuildSimulatorSetRedirect(makeScopedSimulator(paths), { + ...redirectOptions(paths), + backupPath: paths.backupPath, + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.message, 'Failed to redirect XCTest device set path'); + assert.match(String(error.details?.error), /EACCES/); + return true; + }, + ); + assert.equal( + fs.existsSync(paths.lockDirPath), + false, + 'the lock must not outlive the failure', + ); + // The retry put the host's set back, and nothing renamed it aside again on the way out. + assert.equal( + fs.readFileSync(path.join(paths.xctestDeviceSetPath, 'host-device.txt'), 'utf8'), + 'the host owns this', + ); + assert.equal(fs.existsSync(paths.backupPath), false); + } finally { + renameSpy.mockRestore(); + } + }); +}); + test('a restore that was refused on the way in is not reported as a simulator that needs no redirect', async () => { await withTempDir('device-set-leftover-restore-failed-', async (root) => { const paths = makeRedirectPaths(root); @@ -291,7 +344,14 @@ test('a restore that was refused on the way in is not reported as a simulator th ...redirectOptions(paths), backupPath: paths.backupPath, }), - /EACCES/, + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.message, 'Failed to redirect XCTest device set path'); + assert.match(String(error.details?.error), /EACCES/); + assert.match(String(error.details?.restoreError), /EACCES/); + assert.ok(String(error.details?.hint).includes(paths.backupPath)); + return true; + }, ); // The host's set is still renamed aside, which is the fact the caller needs, and the lock went // back anyway so the next acquire does not wait on this one. diff --git a/packages/platform-apple/src/runner/runner-device-set.ts b/packages/platform-apple/src/runner/runner-device-set.ts index 166e3d3cd0..ec57b5922d 100644 --- a/packages/platform-apple/src/runner/runner-device-set.ts +++ b/packages/platform-apple/src/runner/runner-device-set.ts @@ -105,30 +105,33 @@ export async function acquireXcodebuildSimulatorSetRedirect( }); const paths = { xctestDeviceSetPath, backupPath }; + let needsRedirect = false; - // Undo what an earlier build left before anything here decides about this simulator. The same-set - // check below follows symlinks, and an interrupted build leaves `XCTestDevices` symlinked into a - // simulator's requested set: read that as "already the same set" and this run would hand the symlink - // back and the next `xcodebuild` would build against the host's own devices. A restore that could not - // run leaves that check and the rename below equally meaningless, so it stops the redirect as well. + // One try, so the lock cannot be given back and then worked under: the restore of an interrupted + // build's leftovers runs first because the same-set check follows symlinks, and `XCTestDevices` left + // pointing into this simulator's requested set would otherwise read as "already redirected" and hand + // the next build the host's own devices. try { reconcileXcodebuildSimulatorSetRedirect(paths); - } catch { - await handBackOrRaise(paths, lockDirPath, releaseLock); + needsRedirect = !sameResolvedPath(requestedSetPath, xctestDeviceSetPath); + if (needsRedirect) { + installDeviceSetRedirect(paths, requestedSetPath); + } + } catch (error) { + // Anything the hand-back could not undo travels with this report; the lock never outlives the + // failure that ends the acquire. + const handBack = await handBackDeviceSet(paths, lockDirPath, releaseLock); + throw redirectFailure(error, handBack, { requestedSetPath, ...paths }); } - if (sameResolvedPath(requestedSetPath, xctestDeviceSetPath)) { + if (!needsRedirect) { // Nothing is displaced and the caller gets no handle: a lock this simulator never needed must not // arrive as a redirect problem, and a host device set that could not be put back still must. - await handBackOrRaise(paths, lockDirPath, releaseLock); - return null; - } - - try { - installDeviceSetRedirect(paths, requestedSetPath); - } catch (error) { const handBack = await handBackDeviceSet(paths, lockDirPath, releaseLock); - throw redirectFailure(error, handBack, { requestedSetPath, ...paths }); + if (handBack.restoreFailure !== null) { + throw handBack.restoreFailure; + } + return null; } let givenBack = false; @@ -225,7 +228,7 @@ async function handBackDeviceSet( } catch (error) { restoreFailure = error; // Observed here, while the lock is still held and the release has not moved anything. - renamedAsidePath = findDeviceSetBackup(paths.backupPath); + renamedAsidePath = findDeviceSetBackup(paths); } try { await releaseLock(); @@ -237,27 +240,18 @@ async function handBackDeviceSet( } /** - * Hands the device set back for an exit that has no report of its own to make, and raises the restore - * when the host's own set is still not in place. The hand-back records the release, so there is nothing - * here that could read one half of its result and miss the other. + * Where the host's own device set sits when it is not in place: the backup this run would have written, + * or the older name an earlier version used, but only while it is really on disk. Once the host's set is + * back at its own path nothing is renamed aside, and naming a leftover backup would point a reader at a + * copy they could restore over the set in use. */ -async function handBackOrRaise( - paths: DeviceSetPaths, - lockDirPath: string, - releaseLock: () => Promise, -): Promise { - const { restoreFailure } = await handBackDeviceSet(paths, lockDirPath, releaseLock); - if (restoreFailure !== null) { - throw restoreFailure; +function findDeviceSetBackup(paths: DeviceSetPaths): string | null { + const { xctestDeviceSetPath, backupPath } = paths; + if (!isSymlink(xctestDeviceSetPath) && fs.existsSync(xctestDeviceSetPath)) { + // The host's own set is back where it belongs. Any backup still on disk is a leftover of an older + // interruption, and sending a reader to copy it back would overwrite the set that is in place. + return null; } -} - -/** - * The backup that holds the host's own device set, when one is really on disk. The path this run would - * have written is not the only candidate: an older version renamed it beside a different name, and that - * leftover is just as much the host's device set. - */ -function findDeviceSetBackup(backupPath: string): string | null { return ( [backupPath, ...findLegacyXcodebuildSimulatorSetBackups(backupPath)].find((candidate) => fs.existsSync(candidate), From 615c58cc87dec191f4d0ac145d3c1fdd759954a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 16 Sep 2026 17:19:21 +0200 Subject: [PATCH 15/18] test(apple-runner): pin the backup path a half-finished restore may name --- .../__tests__/runner-device-set.test.ts | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts index 657cd41591..0b337d114c 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts @@ -404,6 +404,76 @@ test('the host’s device set is back before the lock is', async () => { }); }); +test('a restore that half-finished names the backup it was refused, not an older leftover', async () => { + await withTempDir('device-set-half-restored-legacy-', async (root) => { + const paths = makeRedirectPaths(root); + // The default backup path, so the older version's leftover prefix is the one a real host sees. + const backupPath = `${paths.xctestDeviceSetPath}.agent-device-backup`; + const legacyBackupPath = path.join( + path.dirname(backupPath), + '.agent-device-xctestdevices-backup-1600000000000', + ); + fs.mkdirSync(paths.requestedSetPath, { recursive: true }); + fs.mkdirSync(backupPath, { recursive: true }); + fs.writeFileSync(path.join(backupPath, 'host-device.txt'), 'the host owns this'); + fs.mkdirSync(legacyBackupPath, { recursive: true }); + fs.writeFileSync(path.join(legacyBackupPath, 'stale-device.txt'), 'an older interruption'); + fs.symlinkSync(paths.requestedSetPath, paths.xctestDeviceSetPath, 'dir'); + + // An interrupted build left the symlink, this run's backup holds the host's set, and an older + // version's leftover sits beside it. The first restore is refused after it took the symlink down, so + // the hand-back is the one that puts the host's set back — and it is refused while deleting the + // leftover. The report must name what is still renamed aside, which by then is nothing: sending the + // reader to the older leftover would have them copy a stale set over the one now in place. + const realRename = fs.renameSync; + let restoreAttempts = 0; + const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation(((from, to) => { + if (String(to) === paths.xctestDeviceSetPath) { + restoreAttempts += 1; + if (restoreAttempts === 1) { + throw Object.assign(new Error('EACCES: permission denied'), { code: 'EACCES' }); + } + } + return realRename(from, to); + }) as typeof fs.renameSync); + const realRm = fs.rmSync; + const rmSpy = vi.spyOn(fs, 'rmSync').mockImplementation(((target, options) => { + if (String(target) === legacyBackupPath) { + throw Object.assign(new Error('EPERM: operation not permitted'), { code: 'EPERM' }); + } + return realRm(target as Parameters[0], options); + }) as typeof fs.rmSync); + + try { + await assert.rejects( + () => + acquireXcodebuildSimulatorSetRedirect(makeScopedSimulator(paths), redirectOptions(paths)), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.message, 'Failed to redirect XCTest device set path'); + assert.match(String(error.details?.error), /EACCES/); + assert.match(String(error.details?.restoreError), /EPERM/); + assert.equal(error.details?.hint, undefined); + return true; + }, + ); + assert.equal(fs.existsSync(legacyBackupPath), true, 'the leftover stays where it is'); + assert.equal( + fs.readFileSync(path.join(paths.xctestDeviceSetPath, 'host-device.txt'), 'utf8'), + 'the host owns this', + ); + assert.equal( + fs.existsSync(paths.lockDirPath), + false, + 'the lock must not outlive the failure', + ); + } finally { + rmSpy.mockRestore(); + renameSpy.mockRestore(); + } + }); +}); + test('a redirect that failed before it moved anything names no backup that is not there', async () => { await withTempDir('device-set-failed-before-rename-', async (root) => { const paths = makeRedirectPaths(root); From 876f74652bab7170df5c76aceb2692ec5740aecd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 16 Sep 2026 18:45:12 +0200 Subject: [PATCH 16/18] refactor: drop the lock leftovers withProcessLock made redundant The Swift recording cache was the one lock-and-run site still choosing by hand which failure to report, and it chose differently from the rest: a release it could not verify was swallowed even after a build that succeeded. It runs under `withProcessLock` now, which is the rule the CHANGELOG already claimed for it. The pre-acquire "already built" check went with it: the caller checks the same thing synchronously one call earlier, and the check inside the task is the one that can actually observe another process's build. The device-set redirect carried three injection options (`ownerPid`, `ownerStartTime`, `nowMs`) that no caller or test ever passed, and a lock wrapper whose overridable parameters were never overridden. The acquire names its lock directly. `RunnerSession.simulatorSetRedirect` is the redirect's own handle type instead of a structural copy of it. The seven device-set tests that lived in the xctestrun test file since the module split moved next to the module's own tests, and the redirect harness that both files had grown is one harness again. --- .../capture-kit/src/recording/swift-cache.ts | 78 ++++--- .../__tests__/runner-device-set.test.ts | 189 ++++++++++++++++ .../runner/__tests__/runner-xctestrun.test.ts | 212 ------------------ .../src/runner/runner-device-set.ts | 37 +-- .../src/runner/runner-session-types.ts | 7 +- 5 files changed, 239 insertions(+), 284 deletions(-) diff --git a/packages/capture-kit/src/recording/swift-cache.ts b/packages/capture-kit/src/recording/swift-cache.ts index a709e3a999..366a2db6b4 100644 --- a/packages/capture-kit/src/recording/swift-cache.ts +++ b/packages/capture-kit/src/recording/swift-cache.ts @@ -6,7 +6,11 @@ import { trimEdgeDashes } from '@agent-device/kernel/collections'; import { AppError } from '@agent-device/kernel/errors'; import { runCmd } from '@agent-device/host-kit/command'; import { readProcessStartTime } from '@agent-device/host-kit/process'; -import { acquireProcessLock } from '@agent-device/host-kit/file'; +import { + acquireProcessLock, + withProcessLock, + type ProcessLockRelease, +} from '@agent-device/host-kit/file'; const SWIFT_CACHE_VERSION = '2'; const LOCK_RETRY_DELAY_MS = 25; @@ -98,50 +102,44 @@ async function ensureSwiftExecutable(params: { const executableDir = path.dirname(params.executablePath); fs.mkdirSync(executableDir, { recursive: true }); - const lockDir = `${params.executablePath}.lock`; - const releaseLock = await acquireSwiftCacheLock( - lockDir, - params.executablePath, - params.timeoutMs ?? 120_000, - ); - if (!releaseLock) { - return; - } - - const tempDir = fs.mkdtempSync( - path.join(executableDir, `.${path.basename(params.executablePath)}.${process.pid}.`), - ); - const tempExecutablePath = path.join(tempDir, path.basename(params.executablePath)); - try { - if (isExecutableFile(params.executablePath)) { - return; - } - const [primarySourcePath] = params.sourcePaths; - if (params.sourceText !== undefined && primarySourcePath && !fs.existsSync(primarySourcePath)) { - fs.mkdirSync(path.dirname(primarySourcePath), { recursive: true }); - fs.writeFileSync(primarySourcePath, params.sourceText); - } - await runCmd('xcrun', ['swiftc', ...params.sourcePaths, '-o', tempExecutablePath], { - timeoutMs: params.timeoutMs ?? 120_000, - env: buildSwiftToolEnv(), - }); - fs.renameSync(tempExecutablePath, params.executablePath); - } finally { - fs.rmSync(tempDir, { recursive: true, force: true }); - // The build's own failure is the reportable fact; an unverified release leaves the - // lock to the stale-clear path rather than displacing it. - await releaseLock().catch(() => undefined); - } + const timeoutMs = params.timeoutMs ?? 120_000; + await withProcessLock({ + acquire: () => acquireSwiftCacheLock(`${params.executablePath}.lock`, timeoutMs), + task: async () => { + // Another process may have published the executable while this one waited for the lock. + if (isExecutableFile(params.executablePath)) { + return; + } + const tempDir = fs.mkdtempSync( + path.join(executableDir, `.${path.basename(params.executablePath)}.${process.pid}.`), + ); + const tempExecutablePath = path.join(tempDir, path.basename(params.executablePath)); + try { + const [primarySourcePath] = params.sourcePaths; + if ( + params.sourceText !== undefined && + primarySourcePath && + !fs.existsSync(primarySourcePath) + ) { + fs.mkdirSync(path.dirname(primarySourcePath), { recursive: true }); + fs.writeFileSync(primarySourcePath, params.sourceText); + } + await runCmd('xcrun', ['swiftc', ...params.sourcePaths, '-o', tempExecutablePath], { + timeoutMs, + env: buildSwiftToolEnv(), + }); + fs.renameSync(tempExecutablePath, params.executablePath); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }, + }); } async function acquireSwiftCacheLock( lockDir: string, - executablePath: string, timeoutMs: number, -): Promise<(() => Promise) | null> { - if (isExecutableFile(executablePath)) { - return null; - } +): Promise { try { return await acquireProcessLock({ lockDirPath: lockDir, diff --git a/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts index 0b337d114c..f2fcba7f86 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts @@ -7,6 +7,7 @@ import type { DeviceInfo } from '@agent-device/kernel/device'; import { mkdtempForTestSync } from './tmp-dir.ts'; import { acquireXcodebuildSimulatorSetRedirect, + resolveXcodebuildSimulatorDeviceSetPath, withXcodebuildSimulatorSetRedirect, } from '../runner-device-set.ts'; @@ -50,6 +51,23 @@ function makeScopedSimulator(paths: RedirectPaths): DeviceInfo { return { ...iosSimulator, simulatorSetPath: paths.requestedSetPath }; } +async function acquireRedirect( + paths: RedirectPaths, + options: Partial[1]> = {}, +): ReturnType { + return await acquireXcodebuildSimulatorSetRedirect(makeScopedSimulator(paths), { + ...redirectOptions(paths), + ...options, + }); +} + +function assertRedirectTargetsRequestedSet(paths: RedirectPaths): void { + assert.equal( + fs.realpathSync.native(paths.xctestDeviceSetPath), + fs.realpathSync.native(paths.requestedSetPath), + ); +} + /** The lock is standing and its record cannot be read, which is what no release can forgive. */ function makeReleaseUnverifiable(paths: RedirectPaths): void { const ownerFilePath = path.join(paths.lockDirPath, 'owner.json'); @@ -574,3 +592,174 @@ test('a redirect that could not be installed reports the redirect, not the faile } }); }); + +test('resolveXcodebuildSimulatorDeviceSetPath uses XCTestDevices under the user home', () => { + assert.equal( + resolveXcodebuildSimulatorDeviceSetPath('/tmp/agent-device-home'), + '/tmp/agent-device-home/Library/Developer/XCTestDevices', + ); +}); + +test('acquireXcodebuildSimulatorSetRedirect swaps XCTestDevices to the requested simulator set', async () => { + let handle: Awaited> | null = null; + await withTempDir('device-set-redirect-', async (root) => { + const paths = makeRedirectPaths(root); + const originalMarkerPath = path.join(root, 'original-marker.txt'); + fs.mkdirSync(paths.requestedSetPath, { recursive: true }); + fs.mkdirSync(paths.xctestDeviceSetPath, { recursive: true }); + fs.writeFileSync( + path.join(paths.xctestDeviceSetPath, 'original.txt'), + originalMarkerPath, + 'utf8', + ); + + handle = await acquireRedirect(paths); + + assert.notEqual(handle, null); + assert.equal(fs.lstatSync(paths.xctestDeviceSetPath).isSymbolicLink(), true); + assertRedirectTargetsRequestedSet(paths); + + await handle?.release(); + handle = null; + + assert.equal(fs.lstatSync(paths.xctestDeviceSetPath).isDirectory(), true); + assert.equal( + fs.readFileSync(path.join(paths.xctestDeviceSetPath, 'original.txt'), 'utf8'), + originalMarkerPath, + ); + }).finally(async () => { + await handle?.release(); + }); +}); + +test('acquireXcodebuildSimulatorSetRedirect is a no-op for simulators without a scoped device set', async () => { + const handle = await acquireXcodebuildSimulatorSetRedirect(iosSimulator); + assert.equal(handle, null); +}); + +test('acquireXcodebuildSimulatorSetRedirect restores stale redirected XCTestDevices before applying a new one', async () => { + let handle: Awaited> | null = null; + await withTempDir('device-set-redirect-', async (root) => { + const paths = makeRedirectPaths(root); + const staleRequestedSetPath = path.join(root, 'stale-requested'); + fs.mkdirSync(paths.requestedSetPath, { recursive: true }); + fs.mkdirSync(staleRequestedSetPath, { 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'); + fs.symlinkSync(staleRequestedSetPath, 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 restores the backup when XCTestDevices is a dangling symlink', async () => { + let handle: Awaited> | null = null; + await withTempDir('device-set-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('device-set-redirect-', async (root) => { + const paths = makeRedirectPaths(root); + fs.mkdirSync(paths.requestedSetPath, { recursive: true }); + fs.mkdirSync(paths.lockDirPath, { recursive: true }); + fs.writeFileSync( + path.join(paths.lockDirPath, 'owner.json'), + JSON.stringify({ pid: 999_999, startTime: null, acquiredAtMs: Date.now() - 60_000 }), + 'utf8', + ); + + handle = await acquireRedirect(paths); + + assert.notEqual(handle, null); + assert.equal(fs.lstatSync(paths.xctestDeviceSetPath).isSymbolicLink(), true); + + await handle?.release(); + handle = null; + + assert.equal(fs.existsSync(paths.lockDirPath), false); + }).finally(async () => { + await handle?.release(); + }); +}); + +test('acquireXcodebuildSimulatorSetRedirect preserves the backup when XCTestDevices is recreated mid-swap', async () => { + const renameSync = fs.renameSync.bind(fs); + let xctestDeviceSetPath = ''; + const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation((oldPath, newPath) => { + if ( + typeof oldPath === 'string' && + typeof newPath === 'string' && + newPath === xctestDeviceSetPath && + oldPath.includes('.agent-device-link-') + ) { + fs.mkdirSync(xctestDeviceSetPath, { recursive: true }); + fs.writeFileSync(path.join(xctestDeviceSetPath, 'collision.txt'), 'collision', 'utf8'); + } + return renameSync(oldPath, newPath); + }); + try { + await withTempDir('device-set-redirect-', async (root) => { + const paths = makeRedirectPaths(root); + xctestDeviceSetPath = paths.xctestDeviceSetPath; + fs.mkdirSync(paths.requestedSetPath, { recursive: true }); + fs.mkdirSync(paths.xctestDeviceSetPath, { recursive: true }); + fs.writeFileSync(path.join(paths.xctestDeviceSetPath, 'original.txt'), 'original', 'utf8'); + + await assert.rejects( + acquireRedirect(paths, { backupPath: paths.backupPath }), + /Failed to redirect XCTest device set path/, + ); + + assert.equal( + fs.readFileSync(path.join(paths.backupPath, 'original.txt'), 'utf8'), + 'original', + ); + assert.equal( + fs.readFileSync(path.join(paths.xctestDeviceSetPath, 'collision.txt'), 'utf8'), + 'collision', + ); + }); + } finally { + renameSpy.mockRestore(); + } +}); diff --git a/packages/platform-apple/src/runner/__tests__/runner-xctestrun.test.ts b/packages/platform-apple/src/runner/__tests__/runner-xctestrun.test.ts index ca2c02023f..a4c3aa7651 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-xctestrun.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-xctestrun.test.ts @@ -25,9 +25,7 @@ const mockRunCmdSync = vi.fn(); import type { DeviceInfo } from '@agent-device/kernel/device'; import { findXctestrun, scoreXctestrunCandidate } from '../runner-artifact.ts'; -import { resolveXcodebuildSimulatorDeviceSetPath } from '../runner-device-set.ts'; import { - acquireXcodebuildSimulatorSetRedirect, ensureXctestrunArtifact, markRunnerXctestrunArtifactBadForRun, prepareXctestrunWithEnv, @@ -52,13 +50,6 @@ const iosDevice: DeviceInfo = { booted: true, }; -type RedirectPaths = { - requestedSetPath: string; - xctestDeviceSetPath: string; - lockDirPath: string; - backupPath: string; -}; - const runnerPortEnv = { AGENT_DEVICE_RUNNER_PORT: '12345' }; function appleToolFingerprintOutput(command: string, args: readonly string[]): string { @@ -117,31 +108,6 @@ async function withTempDir(prefix: string, fn: (root: string) => Promise | } } -function makeRedirectPaths(root: string): RedirectPaths { - const xctestDeviceSetPath = path.join(root, 'Library', 'Developer', 'XCTestDevices'); - return { - requestedSetPath: path.join(root, 'requested'), - xctestDeviceSetPath, - lockDirPath: path.join(root, '.agent-device', 'xctest-device-set.lock'), - backupPath: `${xctestDeviceSetPath}.agent-device-backup`, - }; -} - -function makeScopedSimulator(paths: RedirectPaths): DeviceInfo { - return { ...iosSimulator, simulatorSetPath: paths.requestedSetPath }; -} - -async function acquireRedirect( - paths: RedirectPaths, - options: Partial[1]> = {}, -): ReturnType { - return await acquireXcodebuildSimulatorSetRedirect(makeScopedSimulator(paths), { - lockDirPath: paths.lockDirPath, - xctestDeviceSetPath: paths.xctestDeviceSetPath, - ...options, - }); -} - async function prepareXctestrunJson( xctestrunPath: string, envVars: Record, @@ -164,13 +130,6 @@ function assertNoCapturePolicy(target: any): void { assert.equal(target?.UserAttachmentLifetime, undefined); } -function assertRedirectTargetsRequestedSet(paths: RedirectPaths): void { - assert.equal( - fs.realpathSync.native(paths.xctestDeviceSetPath), - fs.realpathSync.native(paths.requestedSetPath), - ); -} - test('findXctestrun prefers simulator xctestrun over newer macos candidate', () => { const root = mkdtempForTestSync('runner-xctestrun-'); try { @@ -477,174 +436,3 @@ test('markRunnerXctestrunArtifactBadForRun preserves configured external artifac assert.equal(fs.existsSync(xctestrunPath), true); }); }); - -test('resolveXcodebuildSimulatorDeviceSetPath uses XCTestDevices under the user home', () => { - assert.equal( - resolveXcodebuildSimulatorDeviceSetPath('/tmp/agent-device-home'), - '/tmp/agent-device-home/Library/Developer/XCTestDevices', - ); -}); - -test('acquireXcodebuildSimulatorSetRedirect swaps XCTestDevices to the requested simulator set', async () => { - let handle: Awaited> | null = null; - await withTempDir('runner-xctestrun-redirect-', async (root) => { - const paths = makeRedirectPaths(root); - const originalMarkerPath = path.join(root, 'original-marker.txt'); - fs.mkdirSync(paths.requestedSetPath, { recursive: true }); - fs.mkdirSync(paths.xctestDeviceSetPath, { recursive: true }); - fs.writeFileSync( - path.join(paths.xctestDeviceSetPath, 'original.txt'), - originalMarkerPath, - 'utf8', - ); - - handle = await acquireRedirect(paths); - - assert.notEqual(handle, null); - assert.equal(fs.lstatSync(paths.xctestDeviceSetPath).isSymbolicLink(), true); - assertRedirectTargetsRequestedSet(paths); - - await handle?.release(); - handle = null; - - assert.equal(fs.lstatSync(paths.xctestDeviceSetPath).isDirectory(), true); - assert.equal( - fs.readFileSync(path.join(paths.xctestDeviceSetPath, 'original.txt'), 'utf8'), - originalMarkerPath, - ); - }).finally(async () => { - await handle?.release(); - }); -}); - -test('acquireXcodebuildSimulatorSetRedirect is a no-op for simulators without a scoped device set', async () => { - const handle = await acquireXcodebuildSimulatorSetRedirect(iosSimulator); - assert.equal(handle, null); -}); - -test('acquireXcodebuildSimulatorSetRedirect restores stale redirected XCTestDevices before applying a new one', async () => { - let handle: Awaited> | null = null; - await withTempDir('runner-xctestrun-redirect-', async (root) => { - const paths = makeRedirectPaths(root); - const staleRequestedSetPath = path.join(root, 'stale-requested'); - fs.mkdirSync(paths.requestedSetPath, { recursive: true }); - fs.mkdirSync(staleRequestedSetPath, { 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'); - fs.symlinkSync(staleRequestedSetPath, 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 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) => { - const paths = makeRedirectPaths(root); - fs.mkdirSync(paths.requestedSetPath, { recursive: true }); - fs.mkdirSync(paths.lockDirPath, { recursive: true }); - fs.writeFileSync( - path.join(paths.lockDirPath, 'owner.json'), - JSON.stringify({ pid: 999_999, startTime: null, acquiredAtMs: Date.now() - 60_000 }), - 'utf8', - ); - - handle = await acquireRedirect(paths); - - assert.notEqual(handle, null); - assert.equal(fs.lstatSync(paths.xctestDeviceSetPath).isSymbolicLink(), true); - - await handle?.release(); - handle = null; - - assert.equal(fs.existsSync(paths.lockDirPath), false); - }).finally(async () => { - await handle?.release(); - }); -}); - -test('acquireXcodebuildSimulatorSetRedirect preserves the backup when XCTestDevices is recreated mid-swap', async () => { - const renameSync = fs.renameSync.bind(fs); - let xctestDeviceSetPath = ''; - const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation((oldPath, newPath) => { - if ( - typeof oldPath === 'string' && - typeof newPath === 'string' && - newPath === xctestDeviceSetPath && - oldPath.includes('.agent-device-link-') - ) { - fs.mkdirSync(xctestDeviceSetPath, { recursive: true }); - fs.writeFileSync(path.join(xctestDeviceSetPath, 'collision.txt'), 'collision', 'utf8'); - } - return renameSync(oldPath, newPath); - }); - try { - await withTempDir('runner-xctestrun-redirect-', async (root) => { - const paths = makeRedirectPaths(root); - xctestDeviceSetPath = paths.xctestDeviceSetPath; - fs.mkdirSync(paths.requestedSetPath, { recursive: true }); - fs.mkdirSync(paths.xctestDeviceSetPath, { recursive: true }); - fs.writeFileSync(path.join(paths.xctestDeviceSetPath, 'original.txt'), 'original', 'utf8'); - - await assert.rejects( - acquireRedirect(paths, { backupPath: paths.backupPath }), - /Failed to redirect XCTest device set path/, - ); - - assert.equal( - fs.readFileSync(path.join(paths.backupPath, 'original.txt'), 'utf8'), - 'original', - ); - assert.equal( - fs.readFileSync(path.join(paths.xctestDeviceSetPath, 'collision.txt'), 'utf8'), - 'collision', - ); - }); - } finally { - renameSpy.mockRestore(); - } -}); diff --git a/packages/platform-apple/src/runner/runner-device-set.ts b/packages/platform-apple/src/runner/runner-device-set.ts index ec57b5922d..93da73a14b 100644 --- a/packages/platform-apple/src/runner/runner-device-set.ts +++ b/packages/platform-apple/src/runner/runner-device-set.ts @@ -9,7 +9,7 @@ import { readProcessStartTime, acquireProcessLock, withProcessLock, - type ProcessLockOwner, + type ProcessLockRelease, } from './host.ts'; const XCTEST_DEVICE_SET_BASE_NAME = 'XCTestDevices'; @@ -38,9 +38,6 @@ type XcodebuildSimulatorSetRedirectOptions = { xctestDeviceSetPath?: string; backupPath?: string; lockDirPath?: string; - ownerPid?: number; - ownerStartTime?: string | null; - nowMs?: number; }; export function resolveXcodebuildSimulatorDeviceSetPath(homeDir: string = os.homedir()): string { @@ -94,14 +91,17 @@ export async function acquireXcodebuildSimulatorSetRedirect( const lockDirPath = path.resolve( options.lockDirPath ?? resolveXcodebuildSimulatorDeviceSetLockPath(), ); - const ownerStartTime = options.ownerStartTime ?? readProcessStartTime(process.pid); - const releaseLock = await acquireXcodebuildSimulatorSetLock({ + const releaseLock = await acquireProcessLock({ lockDirPath, owner: { - pid: options.ownerPid ?? process.pid, - startTime: ownerStartTime, - acquiredAtMs: options.nowMs ?? Date.now(), + pid: process.pid, + startTime: readProcessStartTime(process.pid), + acquiredAtMs: Date.now(), }, + timeoutMs: XCTEST_DEVICE_SET_LOCK_TIMEOUT_MS, + pollMs: XCTEST_DEVICE_SET_LOCK_POLL_MS, + ownerGraceMs: XCTEST_DEVICE_SET_LOCK_OWNER_GRACE_MS, + description: 'XCTest device set lock', }); const paths = { xctestDeviceSetPath, backupPath }; @@ -217,7 +217,7 @@ type DeviceSetHandBack = { async function handBackDeviceSet( paths: DeviceSetPaths, lockDirPath: string, - releaseLock: () => Promise, + releaseLock: ProcessLockRelease, ): Promise { let restoreFailure: unknown = null; let renamedAsidePath: string | null = null; @@ -390,20 +390,3 @@ function sameResolvedPath(left: string, right: string): boolean { return false; } } - -async function acquireXcodebuildSimulatorSetLock(params: { - lockDirPath: string; - owner: ProcessLockOwner; - timeoutMs?: number; - pollMs?: number; - description?: string; -}): Promise<() => Promise> { - return await acquireProcessLock({ - lockDirPath: params.lockDirPath, - owner: params.owner, - timeoutMs: params.timeoutMs ?? XCTEST_DEVICE_SET_LOCK_TIMEOUT_MS, - pollMs: params.pollMs ?? XCTEST_DEVICE_SET_LOCK_POLL_MS, - ownerGraceMs: XCTEST_DEVICE_SET_LOCK_OWNER_GRACE_MS, - description: params.description ?? 'XCTest device set lock', - }); -} diff --git a/packages/platform-apple/src/runner/runner-session-types.ts b/packages/platform-apple/src/runner/runner-session-types.ts index 25b58cdef3..86a1f91ad8 100644 --- a/packages/platform-apple/src/runner/runner-session-types.ts +++ b/packages/platform-apple/src/runner/runner-session-types.ts @@ -3,6 +3,7 @@ import type { ExecResult } from './host.ts'; import type { DeviceInfo } from '@agent-device/kernel/device'; import type { RunnerXctestrunArtifact } from './runner-xctestrun.ts'; import type { RunnerLease } from './runner-lease.ts'; +import type { XcodebuildSimulatorSetRedirectHandle } from './runner-device-set.ts'; // The runner process seen through the session: pid for liveness/kill-tree and // exitCode for early-exit detection. A spawned ChildProcess satisfies this @@ -50,11 +51,7 @@ export type RunnerSession = { startupTimings?: Record; startupTimingsReported?: boolean; logicalLeaseContext?: RunnerLogicalLeaseContext; - /** `XcodebuildSimulatorSetRedirectHandle`, seen through the two operations a session performs. */ - simulatorSetRedirect?: { - release: () => Promise; - releaseBestEffort: () => Promise; - }; + simulatorSetRedirect?: XcodebuildSimulatorSetRedirectHandle; lease?: RunnerLease; }; From 6aa2f85c8f089a6cc4b0b576bc5f0ac59f064e4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 16 Sep 2026 18:49:37 +0200 Subject: [PATCH 17/18] refactor: the snapshot bridge cache runs under withProcessLock too `ensureSnapshotBridgeBinary` was the one lock-and-run site left releasing in a `finally`, so a build that failed could still be reported as the lock it could not hand back. It goes through `withProcessLock` now, with the lock still taken through the snapshot-source host seam. The CHANGELOG names it in place of "atomic file publishes", which no site in this change was. The runner cache lock loses the same never-overridden wrapper the device-set lock lost, and the malformed-record case in the process-lock tests joins the list of uninformative records it was a copy of. --- CHANGELOG.md | 2 +- .../src/internal/process-lock.test.ts | 24 +-- .../platform-apple/src/runner/runner-cache.ts | 20 +- .../src/snapshot-source/cache.ts | 181 +++++++++--------- 4 files changed, 95 insertions(+), 132 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eebc1de872..f48f2f660c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -160,7 +160,7 @@ handed back, so a build or a publish that failed keeps its own error instead of being replaced by `Timed out waiting for …`, and work that succeeded still reports the lock it could not give back. The Apple runner's artifact, cache, lease and disposal paths, the managed-allocation store, the - device-claim store, atomic file publishes, the Swift recording cache and the agent-browser setup + device-claim store, the iOS snapshot bridge cache, the Swift recording cache and the agent-browser setup moved onto it, replacing hand-written try/catch pairs that each chose differently. - Fixed: the redirect of `~/Library/Developer/XCTestDevices` gives itself back in one order — restore the host's own device set, then release the lock — and a restore that was refused is what the caller diff --git a/packages/host-kit/src/internal/process-lock.test.ts b/packages/host-kit/src/internal/process-lock.test.ts index 58bb487c2b..f1c8e9494f 100644 --- a/packages/host-kit/src/internal/process-lock.test.ts +++ b/packages/host-kit/src/internal/process-lock.test.ts @@ -203,29 +203,6 @@ test('a reacquired lock publishes a claim that its predecessor cannot reuse', as assert.notEqual(firstToken, secondToken); }); -test('acquireProcessLock does not evict a live owner whose owner.json is malformed', async () => { - const lockDirPath = path.join(tmpDir, 'malformed.lock'); - fs.mkdirSync(lockDirPath); - fs.writeFileSync(path.join(lockDirPath, 'owner.json'), '{ pid: '); - stampDirectoryAbandoned(lockDirPath); - - await assert.rejects( - () => - acquireProcessLock({ - lockDirPath, - owner: currentProcessOwner(), - timeoutMs: 50, - pollMs: 1, - }), - (error: unknown) => { - assert.ok(error instanceof AppError); - assert.equal(error.details?.ownerRecordUnreadable, true); - return true; - }, - ); - assert.equal(fs.existsSync(lockDirPath), true); -}); - test('acquireProcessLock does not evict an owner record it cannot read', async () => { const lockDirPath = path.join(tmpDir, 'unreadable.lock'); fs.mkdirSync(lockDirPath); @@ -311,6 +288,7 @@ function listReclaimSiblings(directory: string): string[] { } const UNINFORMATIVE_OWNER_RECORDS = [ + '{ pid: ', 'null', '"999999999"', '{"pid":"999999999","startTime":null,"acquiredAtMs":1}', diff --git a/packages/platform-apple/src/runner/runner-cache.ts b/packages/platform-apple/src/runner/runner-cache.ts index 47f188ac11..501458404c 100644 --- a/packages/platform-apple/src/runner/runner-cache.ts +++ b/packages/platform-apple/src/runner/runner-cache.ts @@ -6,7 +6,6 @@ import { readProcessStartTime, acquireProcessLock, withProcessLock, - type ProcessLockOwner, isEnvTruthy, findProjectRoot, } from './host.ts'; @@ -119,30 +118,17 @@ export async function markRunnerXctestrunArtifactBadForRun( export async function acquireRunnerXctestrunCacheLock( derived: string, ): Promise<() => Promise> { - return await acquireRunnerCacheProcessLock({ + return await acquireProcessLock({ lockDirPath: resolveRunnerXctestrunCacheLockPath(derived), owner: { pid: process.pid, startTime: readProcessStartTime(process.pid), acquiredAtMs: Date.now(), }, - description: 'iOS runner cache lock', - }); -} - -async function acquireRunnerCacheProcessLock(params: { - lockDirPath: string; - owner: ProcessLockOwner; - timeoutMs?: number; - description?: string; -}): Promise<() => Promise> { - return await acquireProcessLock({ - lockDirPath: params.lockDirPath, - owner: params.owner, - timeoutMs: params.timeoutMs ?? RUNNER_XCTESTRUN_CACHE_LOCK_TIMEOUT_MS, + timeoutMs: RUNNER_XCTESTRUN_CACHE_LOCK_TIMEOUT_MS, pollMs: RUNNER_XCTESTRUN_CACHE_LOCK_POLL_MS, ownerGraceMs: RUNNER_XCTESTRUN_CACHE_LOCK_OWNER_GRACE_MS, - description: params.description ?? 'iOS runner cache lock', + description: 'iOS runner cache lock', }); } diff --git a/packages/platform-apple/src/snapshot-source/cache.ts b/packages/platform-apple/src/snapshot-source/cache.ts index 5164050f18..73efeed2c4 100644 --- a/packages/platform-apple/src/snapshot-source/cache.ts +++ b/packages/platform-apple/src/snapshot-source/cache.ts @@ -1,5 +1,6 @@ import { createHash } from 'node:crypto'; import path from 'node:path'; +import { withProcessLock } from '@agent-device/host-kit/file'; import { SnapshotSourceError, snapshotSourceError } from './errors.ts'; import { remainingSnapshotSourceMs, type SnapshotSourceDeadline } from './deadline.ts'; import { @@ -61,103 +62,101 @@ export async function ensureSnapshotBridgeBinary( const cacheRoot = input.cacheRoot ?? path.join(input.host.homeDirectory(), '.agent-device', 'snapshot-source'); const entryPath = path.join(cacheRoot, cacheKey); - const releaseLock = await input.host.acquireLock(path.join(cacheRoot, `${cacheKey}.lock`), { - deadline, - }); - try { - const cached = await readValidCache( - input.host, - entryPath, - { - sourceHash, - cacheKey, - toolchain, - }, - deadline, - ); - if (cached) return cached; - remainingSnapshotSourceMs(deadline, 'native-build-deadline'); - if (input.host.exists(entryPath)) await input.host.remove(entryPath); - - remainingSnapshotSourceMs(deadline, 'native-build-deadline'); - await input.host.ensureDirectory(cacheRoot); - const temporaryPath = path.join(cacheRoot, `.${cacheKey}.${input.host.processId()}.tmp`); - remainingSnapshotSourceMs(deadline, 'native-build-deadline'); - await input.host.remove(temporaryPath); - try { - remainingSnapshotSourceMs(deadline, 'native-build-deadline'); - await input.host.ensureDirectory(temporaryPath); - const outputPath = path.join(temporaryPath, BRIDGE_FILENAME); - const result = await input.host.run( - 'xcrun', - [ - '--sdk', - 'iphonesimulator', - 'clang', - '-arch', - toolchain.architecture, - '-mios-simulator-version-min=15.0', - '-fobjc-arc', - '-Werror', - '-Wall', - '-Wextra', - '-framework', - 'Foundation', - '-framework', - 'CoreGraphics', - ...SNAPSHOT_BRIDGE_COMPILE_FILENAMES.map((sourceFile) => - path.join(sourceRoot, sourceFile), - ), - '-o', - outputPath, - ], + return await withProcessLock({ + acquire: () => input.host.acquireLock(path.join(cacheRoot, `${cacheKey}.lock`), { deadline }), + task: async () => { + const cached = await readValidCache( + input.host, + entryPath, { - signal: deadline.signal, - timeoutMs: Math.min( - BUILD_TIMEOUT_MS, - remainingSnapshotSourceMs(deadline, 'native-build-deadline'), - ), - allowFailure: true, + sourceHash, + cacheKey, + toolchain, }, + deadline, ); - if (result.exitCode !== 0 || !input.host.exists(outputPath)) { - throw snapshotSourceError('unsupported', 'native-build-failed', { - exitCode: result.exitCode, - stderr: result.stderr.slice(0, 4096), - }); - } + if (cached) return cached; remainingSnapshotSourceMs(deadline, 'native-build-deadline'); - await input.host.chmod(outputPath, 0o755); - const binarySha256 = await sha256File(input.host, outputPath, deadline); - const manifest: SnapshotBridgeCacheManifest = { - schemaVersion: CACHE_SCHEMA_VERSION, - protocolVersion: SNAPSHOT_SOURCE_PROTOCOL_VERSION, - sourceVersion: SNAPSHOT_SOURCE_VERSION, - sourceHash, - cacheKey, - toolchain, - binarySha256, - }; - await input.host.writeText( - path.join(temporaryPath, MANIFEST_FILENAME), - `${JSON.stringify(manifest, null, 2)}\n`, - ); + if (input.host.exists(entryPath)) await input.host.remove(entryPath); + + remainingSnapshotSourceMs(deadline, 'native-build-deadline'); + await input.host.ensureDirectory(cacheRoot); + const temporaryPath = path.join(cacheRoot, `.${cacheKey}.${input.host.processId()}.tmp`); remainingSnapshotSourceMs(deadline, 'native-build-deadline'); - await input.host.rename(temporaryPath, entryPath); - return { - path: path.join(entryPath, BRIDGE_FILENAME), - sourceHash, - cacheKey, - protocolVersion: SNAPSHOT_SOURCE_PROTOCOL_VERSION, - sourceVersion: SNAPSHOT_SOURCE_VERSION, - }; - } catch (error) { await input.host.remove(temporaryPath); - throw error; - } - } finally { - await releaseLock(); - } + try { + remainingSnapshotSourceMs(deadline, 'native-build-deadline'); + await input.host.ensureDirectory(temporaryPath); + const outputPath = path.join(temporaryPath, BRIDGE_FILENAME); + const result = await input.host.run( + 'xcrun', + [ + '--sdk', + 'iphonesimulator', + 'clang', + '-arch', + toolchain.architecture, + '-mios-simulator-version-min=15.0', + '-fobjc-arc', + '-Werror', + '-Wall', + '-Wextra', + '-framework', + 'Foundation', + '-framework', + 'CoreGraphics', + ...SNAPSHOT_BRIDGE_COMPILE_FILENAMES.map((sourceFile) => + path.join(sourceRoot, sourceFile), + ), + '-o', + outputPath, + ], + { + signal: deadline.signal, + timeoutMs: Math.min( + BUILD_TIMEOUT_MS, + remainingSnapshotSourceMs(deadline, 'native-build-deadline'), + ), + allowFailure: true, + }, + ); + if (result.exitCode !== 0 || !input.host.exists(outputPath)) { + throw snapshotSourceError('unsupported', 'native-build-failed', { + exitCode: result.exitCode, + stderr: result.stderr.slice(0, 4096), + }); + } + remainingSnapshotSourceMs(deadline, 'native-build-deadline'); + await input.host.chmod(outputPath, 0o755); + const binarySha256 = await sha256File(input.host, outputPath, deadline); + const manifest: SnapshotBridgeCacheManifest = { + schemaVersion: CACHE_SCHEMA_VERSION, + protocolVersion: SNAPSHOT_SOURCE_PROTOCOL_VERSION, + sourceVersion: SNAPSHOT_SOURCE_VERSION, + sourceHash, + cacheKey, + toolchain, + binarySha256, + }; + await input.host.writeText( + path.join(temporaryPath, MANIFEST_FILENAME), + `${JSON.stringify(manifest, null, 2)}\n`, + ); + remainingSnapshotSourceMs(deadline, 'native-build-deadline'); + await input.host.rename(temporaryPath, entryPath); + return { + path: path.join(entryPath, BRIDGE_FILENAME), + sourceHash, + cacheKey, + protocolVersion: SNAPSHOT_SOURCE_PROTOCOL_VERSION, + sourceVersion: SNAPSHOT_SOURCE_VERSION, + }; + } catch (error) { + await input.host.remove(temporaryPath); + throw error; + } + }, + }); } function resolveSnapshotBridgeSourceRoot(host: SnapshotSourceHost): string { From f71d2e5235dd9754ea61f12f476a6a3705386fd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 17 Sep 2026 07:34:21 +0200 Subject: [PATCH 18/18] test: a failed build outranks an unverifiable lock release at both artifact caches One test each for the Swift recorder cache and the snapshot bridge cache: the compile fails while the lock's record is made unreadable under it, and the caller hears the build failure with the lock left standing. Reverting the precedence in `withProcessLock` turns exactly these two red. --- .../src/recording/swift-cache.test.ts | 26 +++++++++ .../src/snapshot-source/cache.test.ts | 55 ++++++++++++++++++- 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/packages/capture-kit/src/recording/swift-cache.test.ts b/packages/capture-kit/src/recording/swift-cache.test.ts index f3d765ae14..f77b318d3c 100644 --- a/packages/capture-kit/src/recording/swift-cache.test.ts +++ b/packages/capture-kit/src/recording/swift-cache.test.ts @@ -162,6 +162,32 @@ test('compileSwiftSourceText falls back to swift-helper when the cache name sani expect(fs.statSync(executablePath).mode & 0o111).not.toBe(0); }); +test('a compile that failed is reported over a cache lock that could not be given back', async () => { + const sourcePath = writeSourceFile(); + const buildFailure = new Error('swiftc: error: build failed'); + let lockDir = ''; + mockRunCmd.mockImplementationOnce(async (_cmd: string, args: string[]) => { + // The temp executable sits one directory under the cache entry, and the lock beside it. + const outputPath = args[args.indexOf('-o') + 1]!; + const executablePath = path.join( + path.dirname(path.dirname(outputPath)), + path.basename(outputPath), + ); + lockDir = `${executablePath}.lock`; + // A record that cannot be read is a release that cannot prove ownership. + const ownerFile = path.join(lockDir, 'owner.json'); + fs.rmSync(ownerFile); + fs.mkdirSync(ownerFile); + throw buildFailure; + }); + + await expect(compileSwiftSourceFile({ sourcePath, cacheName: 'recording-overlay' })).rejects.toBe( + buildFailure, + ); + // The release really could not verify itself: the lock is still standing. + expect(fs.existsSync(lockDir)).toBe(true); +}); + function writeSourceFile(source = 'print("recording")'): string { const sourcePath = path.join(tmpDir, 'recording-overlay.swift'); fs.writeFileSync(sourcePath, source); diff --git a/packages/platform-apple/src/snapshot-source/cache.test.ts b/packages/platform-apple/src/snapshot-source/cache.test.ts index 8f4ee65d67..d381c0ef02 100644 --- a/packages/platform-apple/src/snapshot-source/cache.test.ts +++ b/packages/platform-apple/src/snapshot-source/cache.test.ts @@ -1,10 +1,11 @@ import assert from 'node:assert/strict'; -import { readFile, rm, writeFile } from 'node:fs/promises'; +import { mkdir, readdir, readFile, rm, writeFile } from 'node:fs/promises'; import path from 'node:path'; import { test } from 'vitest'; import { createSnapshotSourceHost } from './host.ts'; import { ensureSnapshotBridgeBinary } from './cache.ts'; +import { SnapshotSourceError } from './errors.ts'; import { createSnapshotSourceDeadline } from './deadline.ts'; import { DEFAULT_SNAPSHOT_SOURCE_LIMITS } from './limits.ts'; import type { SnapshotSourceHost } from './types.ts'; @@ -212,6 +213,58 @@ test('an aborted cache waiter does not cancel an independent preparation', async } }); +test('a bridge build that failed is reported over a cache lock that could not be given back', async () => { + const root = await mkdtempForTest('agent-device-snapshot-source-build-failure-'); + const sourceRoot = path.join(root, 'source'); + const cacheRoot = path.join(root, 'cache'); + await (await import('@agent-device/host-kit/host-file')).ensureHostDirectory(sourceRoot); + await writeFile(path.join(sourceRoot, 'SnapshotBridge.m'), 'native source'); + await writeFile(path.join(sourceRoot, 'SnapshotBridgeRuntime.m'), 'native runtime'); + await writeFile(path.join(sourceRoot, 'SnapshotBridgeRuntime.h'), 'native header'); + await writeFile(path.join(sourceRoot, 'SnapshotBridgeCapture.h'), 'native header'); + await writeFile(path.join(sourceRoot, 'SnapshotBridgeCapture.m'), 'native header'); + const buildHost = createFakeBuildHost('unused'); + const host: SnapshotSourceHost = { + ...buildHost, + run: async (command, args, options) => { + if (command !== 'xcrun' || !args.includes('clang')) { + return await buildHost.run(command, args, options); + } + // The lock beside the cache entry loses its record while the build runs, so the release + // that follows cannot prove ownership; the build itself fails. + const lockDir = (await readdir(cacheRoot)).find((entry) => entry.endsWith('.lock')); + assert.ok(lockDir, 'the build runs under the cache lock'); + const ownerFile = path.join(cacheRoot, lockDir, 'owner.json'); + await rm(ownerFile); + await mkdir(ownerFile); + return { stdout: '', stderr: 'clang: error: build failed', exitCode: 1 }; + }, + }; + + try { + await assert.rejects( + ensureSnapshotBridgeBinary({ + host, + runtime: 'iOS 26.2', + limits: DEFAULT_SNAPSHOT_SOURCE_LIMITS, + deadline: testDeadline(), + sourceRoot, + cacheRoot, + }), + (error: unknown) => { + assert.ok(error instanceof SnapshotSourceError); + assert.equal(error.failureKind, 'unsupported'); + assert.equal(error.failureCode, 'native-build-failed'); + return true; + }, + ); + // The release really could not verify itself: the lock is still standing. + assert.ok((await readdir(cacheRoot)).some((entry) => entry.endsWith('.lock'))); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + async function expectRejectedCancellation(value: Promise): Promise { await assert.rejects(value, (error: unknown) => { return (