diff --git a/CHANGELOG.md b/CHANGELOG.md index ba3e951e30..f48f2f660c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -147,6 +147,31 @@ 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`. + 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, 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 + 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. 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/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/capture-kit/src/recording/swift-cache.ts b/packages/capture-kit/src/recording/swift-cache.ts index ad382f5a8b..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,48 +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 }); - await releaseLock(); - } + 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/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 5e1d393147..f1c8e9494f 100644 --- a/packages/host-kit/src/internal/process-lock.test.ts +++ b/packages/host-kit/src/internal/process-lock.test.ts @@ -11,7 +11,12 @@ 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'; @@ -142,6 +147,671 @@ 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('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 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); +}); + +// 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); + stampDirectoryAbandoned(lockDirPath); + + const attempts = await Promise.allSettled([ + acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + ownerGraceMs: 1_000, + timeoutMs: 250, + pollMs: 2, + }), + acquireProcessLock({ + lockDirPath, + owner: currentProcessOwner(), + ownerGraceMs: 1_000, + 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(listReclaimSiblings(tmpDir), []); +}); + +/** 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('.reclaim')) + .sort(); +} + +const UNINFORMATIVE_OWNER_RECORDS = [ + '{ pid: ', + '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); +}); + +// 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); +}); + +// 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'); + 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 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); + + // 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(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, + // 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); + + 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.ppid); + return true; + }, + ); + assert.equal(claimed, true); + const record = JSON.parse(fs.readFileSync(ownerFilePath, 'utf8')) as { + pid: number; + claimToken: string; + }; + assert.equal(record.pid, process.ppid); + assert.equal(record.claimToken, 'contender-claim'); + } finally { + mkdirSpy.mockRestore(); + } +}); + +// 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); + stampDirectoryAbandoned(lockDirPath); + + // 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); + } + published = true; + fs.writeFileSync( + ownerFilePath, + // 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); + + 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(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 { + mkdirSpy.mockRestore(); + } +}); + +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); + stampDirectoryAbandoned(lockDirPath); + + // 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(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( + () => + acquireProcessLock({ + lockDirPath, + owner: { pid: 999_999_998, startTime: null, acquiredAtMs: Date.now() }, + timeoutMs: 50, + pollMs: 1, + }), + (error: unknown) => { + assert.ok(error instanceof AppError); + return true; + }, + ); + assert.equal(replaced, true); + assert.equal( + fs.statSync(lockDirPath).mtimeMs, + refilledAtMs, + 'the reclaim removed a directory it had not judged abandoned', + ); + } 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 { + mkdirSpy.mockRestore(); + } + assert.ok(lockAttempts > 1, `contender polled ${lockAttempts} times`); + assert.equal(fs.existsSync(ownerFilePath), true); +}); + +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); + + 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 { + 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 { + 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); +} + 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..19941445a0 100644 --- a/packages/host-kit/src/internal/process-lock.ts +++ b/packages/host-kit/src/internal/process-lock.ts @@ -1,13 +1,18 @@ +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 { emitDiagnostic } from './diagnostics.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; +const LOCK_DIRECTORY_SUFFIX = '.lock'; +const RECLAIM_MUTEX_SUFFIX = '.reclaim'; export type ProcessLockOwner = { pid: number; @@ -15,6 +20,67 @@ 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; + /** + * 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. */ +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 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; + 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' } + | { 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(); + +/** 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; @@ -22,25 +88,49 @@ export async function acquireProcessLock(params: { pollMs?: number; ownerGraceMs?: number; description?: string; -}): Promise<() => 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; const description = params.description ?? 'process lock'; fs.mkdirSync(path.dirname(lockDirPath), { recursive: true }); + const claimToken = crypto.randomUUID(); + const claim: ProcessLockOwnerRecord = { ...owner, claimToken, claimIssuerId: CLAIM_ISSUER_ID }; while (Date.now() < deadline) { try { fs.mkdirSync(lockDirPath); - writeProcessLockOwner(ownerFilePath, owner); + writeProcessLockOwner(ownerFilePath, claim); + liveClaimTokens.add(claimToken); let released = false; return async () => { if (released) return; - released = true; - fs.rmSync(lockDirPath, { recursive: true, force: true }); + // 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; + 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. + 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, + hint: staleLockHint(lockDirPath), + }); }; } catch (error) { const err = error as NodeJS.ErrnoException; @@ -54,74 +144,331 @@ 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 writeProcessLockOwner(ownerFilePath: string, owner: ProcessLockOwner): void { +function staleLockHint(lockDirPath: string): string { + return `Remove ${lockDirPath} once you have confirmed no live process holds it, then retry.`; +} + +function writeProcessLockOwner(ownerFilePath: string, owner: ProcessLockOwnerRecord): void { publishFileSync({ destination: ownerFilePath, contents: JSON.stringify(owner), }); } +/** + * 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, + claim: ProcessLockOwnerRecord, +): 'removed' | 'not-owner' | 'unverified' { + const reading = readProcessLockOwner(ownerFilePath); + 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, 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'; + 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( 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)) { - return false; + // 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) && + reclaimLockUnderMutex(lockDirPath, ownerFilePath, ownerGraceMs, { kind: 'stray' }) + ); + } + + 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. 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) || isSpentOwnClaim(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 + // own age speak for it. + if (reading.kind === 'unreadable') { + return false; + } + return ( + reclaimWhenAbandoned(lockStats, ownerGraceMs) && + reclaimLockUnderMutex(lockDirPath, ownerFilePath, ownerGraceMs, { kind: 'empty' }) + ); +} + +function reclaimWhenAbandoned(lockStats: fs.Stats, ownerGraceMs: number): boolean { + return Date.now() - lockStats.mtimeMs >= ownerGraceMs; +} + +/** + * 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 reclaimLockUnderMutex( + lockDirPath: string, + ownerFilePath: string, + ownerGraceMs: number, + judged: JudgedLock, +): boolean { + if (!holdReclaimMutex(lockDirPath, ownerGraceMs)) return false; + try { + 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); + } +} + +/** + * 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 removeDeadClaimLock( + lockDirPath: string, + ownerFilePath: string, + claimToken: ProcessLockOwnerRecord['claimToken'], +): boolean { + const reading = readProcessLockOwner(ownerFilePath); + if (reading.kind !== 'owner' || reading.owner.claimToken !== claimToken) return false; + try { fs.rmSync(lockDirPath, { recursive: true, force: true }); return true; + } catch { + return false; } - if (Date.now() - ownerStats.mtimeMs < ownerGraceMs) { +} + +/** + * 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; + } +} + +/** + * `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 removeStrayLockPath(lockDirPath: string): boolean { + try { + fs.unlinkSync(lockDirPath); + return true; + } catch { return false; } - fs.rmSync(lockDirPath, { recursive: true, force: true }); +} + +/** 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; +} + +/** + * 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 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.rmdirSync(mutexPath); + } catch {} return true; } -function readProcessLockOwner(ownerFilePath: string): ProcessLockOwner | null { +function releaseReclaimMutex(lockDirPath: string): void { + try { + fs.rmdirSync(reclaimMutexPath(lockDirPath)); + } catch {} +} + +function errorCode(error: unknown): string | undefined { + return (error as NodeJS.ErrnoException | null)?.code; +} + +/** + * `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): ProcessLockOwnerRecord | 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, + // 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, + }; } +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 } + : {}), }; } @@ -129,3 +476,17 @@ 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 { + 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); +} diff --git a/packages/managed-allocation/src/__tests__/store.test.ts b/packages/managed-allocation/src/__tests__/store.test.ts index 6dd07be6e9..c0c1e1b0da 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 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}.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 })); + } + + 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..e08ef91955 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, 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()) { return [ 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-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-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 new file mode 100644 index 0000000000..f2fcba7f86 --- /dev/null +++ b/packages/platform-apple/src/runner/__tests__/runner-device-set.test.ts @@ -0,0 +1,765 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +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, + resolveXcodebuildSimulatorDeviceSetPath, + 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; + backupPath: string; + lockDirPath: string; +}; + +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'), + }; +} + +function redirectOptions(paths: RedirectPaths) { + return { + lockDirPath: paths.lockDirPath, + xctestDeviceSetPath: paths.xctestDeviceSetPath, + }; +} + +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'); + 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 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(); + } + }); +}); + +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('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 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); + 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, + }), + (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. + 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); + 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 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); + 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); + 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, + // 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((( + 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, + 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; + }, + ); + 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(); + } + }); +}); + +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-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/__tests__/runner-request-cancellation.test.ts b/packages/platform-apple/src/runner/__tests__/runner-request-cancellation.test.ts index c6f0f45932..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,9 +154,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); 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..6aef63ff4c 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,15 @@ export function makeClassifyOwnerLivenessViaMocks(deps: { return stateDir ? classifyStateDir(stateDir) : 'live'; }; } + +/** + * 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 () => {}); + +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 8f0691988e..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,7 +137,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); @@ -189,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 }; + return redirectHandle; }); const starting = ensureRunnerSession(device, { speculative: true }); @@ -215,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 }; + 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 d53ebc43aa..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,7 +145,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.test.ts b/packages/platform-apple/src/runner/__tests__/runner-session.test.ts index 3b1596492d..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,7 +192,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); @@ -700,7 +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 }); + mockAcquireXcodebuildSimulatorSetRedirect.mockResolvedValue(redirectHandle); mockRunCmdBackground.mockReturnValue(makeBackgroundRunner(4242)); mockWaitForRunner.mockResolvedValue(runnerResponse({ uptimeMs: 1 })); @@ -851,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 () => { @@ -1465,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 () => { @@ -1504,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 { @@ -1546,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); }); 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/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..501458404c 100644 --- a/packages/platform-apple/src/runner/runner-cache.ts +++ b/packages/platform-apple/src/runner/runner-cache.ts @@ -5,7 +5,7 @@ import { emitDiagnostic, readProcessStartTime, acquireProcessLock, - type ProcessLockOwner, + withProcessLock, isEnvTruthy, findProjectRoot, } from './host.ts'; @@ -101,47 +101,34 @@ 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( 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/runner/runner-device-set.ts b/packages/platform-apple/src/runner/runner-device-set.ts index f9d8176b88..93da73a14b 100644 --- a/packages/platform-apple/src/runner/runner-device-set.ts +++ b/packages/platform-apple/src/runner/runner-device-set.ts @@ -8,7 +8,8 @@ import { emitDiagnostic, readProcessStartTime, acquireProcessLock, - type ProcessLockOwner, + withProcessLock, + type ProcessLockRelease, } from './host.ts'; const XCTEST_DEVICE_SET_BASE_NAME = 'XCTestDevices'; @@ -18,17 +19,25 @@ 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 = { + /** + * 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; + /** + * 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; }; type XcodebuildSimulatorSetRedirectOptions = { xctestDeviceSetPath?: string; backupPath?: string; lockDirPath?: string; - ownerPid?: number; - ownerStartTime?: string | null; - nowMs?: number; }; export function resolveXcodebuildSimulatorDeviceSetPath(homeDir: string = os.homedir()): string { @@ -45,6 +54,22 @@ 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 }); +} + export async function acquireXcodebuildSimulatorSetRedirect( device: DeviceInfo, options: XcodebuildSimulatorSetRedirectOptions = {}, @@ -66,67 +91,193 @@ 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 }; + let needsRedirect = false; + + // 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({ - xctestDeviceSetPath, - backupPath, - }); - if (sameResolvedPath(requestedSetPath, xctestDeviceSetPath)) { - await releaseLock(); - return null; + reconcileXcodebuildSimulatorSetRedirect(paths); + 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 }); + } - fs.mkdirSync(requestedSetPath, { recursive: true }); - if (fs.existsSync(xctestDeviceSetPath)) { - fs.renameSync(xctestDeviceSetPath, backupPath); + 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. + const handBack = await handBackDeviceSet(paths, lockDirPath, releaseLock); + if (handBack.restoreFailure !== null) { + throw handBack.restoreFailure; } - installXcodebuildSimulatorSetSymlink({ - requestedSetPath, - xctestDeviceSetPath, - }); - } catch (error) { - reconcileXcodebuildSimulatorSetRedirect({ - xctestDeviceSetPath, - backupPath, - }); - await releaseLock(); - throw new AppError('COMMAND_FAILED', 'Failed to redirect XCTest device set path', { - requestedSetPath, - xctestDeviceSetPath, - backupPath, - error: String(error), - }); + return null; } - let released = false; + let givenBack = false; + const giveBack = async (reportUnverifiedRelease: boolean): Promise => { + if (givenBack) { + return; + } + givenBack = true; + const handBack = await handBackDeviceSet(paths, lockDirPath, releaseLock); + if (handBack.restoreFailure !== null) { + throw handBack.restoreFailure; + } + if (handBack.releaseFailure !== null && reportUnverifiedRelease) { + throw handBack.releaseFailure; + } + }; return { - release: async () => { - if (released) { - return; - } - released = true; - try { - reconcileXcodebuildSimulatorSetRedirect({ - xctestDeviceSetPath, - backupPath, - }); - } finally { - await releaseLock(); - } - }, + release: () => giveBack(true), + releaseBestEffort: () => giveBack(false), }; } +/** 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. */ + 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, 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: DeviceSetPaths, + lockDirPath: string, + releaseLock: ProcessLockRelease, +): 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); + } + try { + await releaseLock(); + } catch (error) { + releaseFailure = error; + } + recordReleaseFailure(releaseFailure, lockDirPath); + return { restoreFailure, renamedAsidePath, releaseFailure }; +} + +/** + * 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. + */ +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; + } + return ( + [backupPath, ...findLegacyXcodebuildSimulatorSetBackups(backupPath)].find((candidate) => + fs.existsSync(candidate), + ) ?? null + ); +} + +/** + * 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 recordReleaseFailure(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; +} + // fallow-ignore-next-line complexity function reconcileXcodebuildSimulatorSetRedirect(paths: { xctestDeviceSetPath: string; @@ -239,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-disposal.ts b/packages/platform-apple/src/runner/runner-disposal.ts index 605abc1c3b..20c3694713 100644 --- a/packages/platform-apple/src/runner/runner-disposal.ts +++ b/packages/platform-apple/src/runner/runner-disposal.ts @@ -191,7 +191,7 @@ async function cleanupRunnerSessionResources( await settleOwnedRunnerDeviceState(session, options); cleanupTempFile(session.xctestrunPath); cleanupTempFile(session.jsonPath); - await session.simulatorSetRedirect?.release(); + await session.simulatorSetRedirect?.releaseBestEffort(); } /** diff --git a/packages/platform-apple/src/runner/runner-lease.ts b/packages/platform-apple/src/runner/runner-lease.ts index a5bf3a3438..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,23 +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 { - return await task(); - } finally { - await release(); - } } function readRunnerLease(deviceId: string): RunnerLease | null { diff --git a/packages/platform-apple/src/runner/runner-session-types.ts b/packages/platform-apple/src/runner/runner-session-types.ts index 7e067057bc..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,7 +51,7 @@ export type RunnerSession = { startupTimings?: Record; startupTimingsReported?: boolean; logicalLeaseContext?: RunnerLogicalLeaseContext; - simulatorSetRedirect?: { release: () => Promise }; + simulatorSetRedirect?: XcodebuildSimulatorSetRedirectHandle; lease?: RunnerLease; }; diff --git a/packages/platform-apple/src/runner/runner-session.ts b/packages/platform-apple/src/runner/runner-session.ts index 0222d514ca..6c77ca4a8a 100644 --- a/packages/platform-apple/src/runner/runner-session.ts +++ b/packages/platform-apple/src/runner/runner-session.ts @@ -248,7 +248,7 @@ async function startRunnerSessionWithLease( }), ); } catch (error) { - await simulatorSetRedirect?.release(); + await simulatorSetRedirect?.releaseBestEffort(); throw error; } const sessionId = buildRunnerSessionId(device.id, port); 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 ( 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 { 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.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 29c419fdba..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,41 +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, - }); - return getManagedAgentBrowserStatus(options); - } finally { - await release(); - } } 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 d86f3d07b2..219e548b5e 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,23 @@ 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*\(/); +}); + +// 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'); + assert.doesNotMatch(source, /renameSync|asidePath|\.reclaimed-/); + assert.match(source, /fs\.rmdirSync/); +}); + 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), diff --git a/src/daemon/device-claim-store.ts b/src/daemon/device-claim-store.ts index 026cd51d27..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,15 +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 { - return await task(); - } finally { - await release(); - } }