fix(host-kit): process lock proves ownership before release and never evicts a live owner

[thymikee]

Summary

acquireProcessLock released its lock directory unconditionally, so a contender that reclaimed the lock
while the previous holder was away had that holder delete the exclusion belonging to whoever holds it
now. Release now removes only while owner.json still names the acquirer and reports a typed
ownerReleaseUnverified otherwise; an unreadable record is an owner of unknown identity, not an absent
one, and no longer ages out at the grace window. A reclaim re-decides in place, and a record naming this
live process is dead once the claim inside it is spent — claims are issued per acquisition and stamped
with the loading of this module that issued them, so a second bundled copy cannot have a live claim
cleared under it.

Lock-and-run callers stop answering "which of my two failures does the caller hear" by hand: the work
inside the lock outranks a lock that could not be handed back, and work that succeeded still reports it.
That moves the Apple runner's artifact, cache, lease, session and disposal paths, the
managed-allocation and device-claim stores, atomic publishes, the Swift recording cache and the
agent-browser setup onto withProcessLock, and so changes which error those callers report. The XCTest
device-set redirect adds the order inside its own give-back: restore, then release, restore first.

Closes #2523. 32 files across host-kit, managed-allocation, capture-kit, platform-apple,
platform-web and the daemon.

Validation

Rebased on origin/main at e3cbc91a1a; tested at 868d0ad713. pnpm check:affected --run passed: 622
test files, 4648 tests, typecheck, lint, layering and Fallow clean; the two scripts/ gates it does not
select pass locally.

Mutations, one at a time, each red only where claimed:

Mutation	Result
drop the spent-claim disjunct from the reclaim	the next acquire waits out its whole timeout
drop the issuer check from the spent-claim rule	another loading's live claim is cleared as spent
restore the finally that let the release outrank the restore	the double-fault test fails through both doors
put the no-redirect release back inside the try	"Failed to redirect XCTest device set path" returns
let releaseBestEffort swallow every error	the EACCES restore test goes quiet

Gaps. The wedge needs a refused unlink, so it is fault-injected: no device run yields EACCES on its
own, and the live coverage is the CI lanes that build and launch through this redirect. The two-loading
case is pinned with a foreign issuer id, not a second real bundle. No win32 evidence here.

[github-actions]

Size Report

Metric	Base	Current	Diff
Installed (including dependencies)	4.57 MB	4.57 MB	+3.7 kB
Package (unpacked)	4.56 MB	4.57 MB	+3.7 kB
Package (download)	1.36 MB	1.36 MB	+1.1 kB

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	29.9 ms	29.6 ms	-0.3 ms
CLI --help	81.7 ms	80.7 ms	-1.0 ms

[thymikee]

Release now checks ownership, but at 2ec6027 a stale reclaim can still give the lock to two contenders. In clearStaleProcessLock, B reads the dead owner's record and decides to reclaim. Before B renames, A renames the stale directory aside, runs mkdir and writes its record. B's renameSync(lockDirPath, asidePath) then moves A's live directory away, and B's mkdir succeeds too. Both now hold the lock, which is what #2523 asks to prevent. After the rename, please confirm that the moved directory is the one you judged stale (for example, compare its inode with the statSync taken before the decision, or re-read the moved record), and put it back if it is not. A test that publishes a live record between the read and the rename would cover it.

Release can now reject with ownerReleaseUnverified, and one caller does not handle that. In packages/platform-apple/src/snapshot-source/host.ts, the cancel path runs void pending.then((release) => release(), () => undefined). The second handler catches only a failed acquire, so a rejected release becomes an unhandled rejection, and the daemon's unhandledRejection handler shuts the daemon down. Please catch it there, and check the callers that release in finally, because a release error now replaces their task result.

A smaller-design question: could each acquisition write a random token into owner.json? Release could compare that token instead of pid and start time, and reclaim could check the token of the directory it moved, which would also close the race above.

Coverage fails because of this change: src/daemon/__tests__/atomic-publish-ownership.test.ts lists process-lock.ts as a simple publisher and rejects fs.renameSync(. The rename is a directory reclaim, not a file publish, so please decide at that rule whether the code or the rule should change. Smoke Tests were still running, and there are no conflicts.

[thymikee]

All four points addressed. The reclaim race and the release callers in 7d4b8f3, the claim token in 9ccc2bc.

The rename race. After the rename, what arrived is now compared against what was judged: the inode and device from the statSync taken before the decision, and the record re-read from the moved directory. Anything that turns out to name a live owner, or is not the inode we judged, is renamed back rather than removed, and the contender goes back to polling. a live owner published between the stale read and the rename keeps its lock publishes a live record from inside a fs.renameSync spy, which is the window you described; the live owner is still in place at the lock path afterwards and no .reclaimed- directory survives. Removing the verification turns that test red.

Release callers. The cancel path in packages/platform-apple/src/snapshot-source/host.ts now releases with release().catch(() => undefined) on the abandoned branch, so a release that cannot prove ownership no longer reaches the daemon's unhandledRejection handler. I read that branch's contract as "the task is gone, the lock goes to the stale-clear path", which is why it is swallowed rather than logged. The abandoned-acquire case has no test harness today, so that one is reasoned rather than covered.

The finally sites were a real problem, as you expected: a release error replaced the task result. withDeviceClaimLock, withRunnerLeaseLock, setupManagedAgentBrowser, ensureSwiftExecutable and acquireXcodebuildSimulatorSetRedirect now keep the task's own failure as the reportable one, and report the unverifiable release only when the task succeeded — a lock that is still standing resolves itself through stale-clear, while nothing else recovers why the task failed. runner-lease-release-ownership.test.ts pins both directions; reverting to finally { await release() } turns the task-failure test red.

The claim token. Adopted, and it turned out to close a hole the inode check left. Each acquisition writes a random claimToken into owner.json; release matches the token instead of pid plus start time alone, and the reclaim compares the token inside the moved directory with the one it judged. The case my inode check missed: a contender reclaims, mkdirs, and has not published yet — same path, no record, and if the inode came back the same the identity check would have been satisfied and I would have deleted a lock that was never mine. A token that does not match refuses it. release leaves a lock that a new acquisition of the same process republished covers release against a record with our pid and start time but a different claim, and is red without the comparison; a reacquired lock publishes a claim that its predecessor cannot reuse pins that consecutive acquisitions differ. A record written before tokens exist reads as an unmatched claim: release never clears it, and reclaim judges it by its owner's liveness as before. The reclaim-side token comparison has no test of its own because inode reuse cannot be provoked hermetically; it is defense in depth behind the identity and liveness checks, both of which are covered.

The coverage rule. I changed the rule. packages/host-kit/src/internal/process-lock.ts was listed among the simple same-directory publishers, and that framing is what broke: publishing a file into a directory and reclaiming a lock directory are different claims of ownership, and the rename is the second one. In src/daemon/__tests__/atomic-publish-ownership.test.ts the file is no longer a simple publisher, and two narrower assertions say what still holds there — its owner record goes through publishFileSync and it writes no file by hand, and every fs.renameSync is addressed between the lock path and the reclaimed name, checked against the actual call sites. It cannot go back to hand-publishing files, and it cannot quietly start renaming something unrelated to the lock.

Gate at 9ccc2bc: pnpm check:affected --run passed, 605 test files. The same gate passed at 7d4b8f3; the first run there failed check:fallow on reclaimProcessLockDirectory at cyclomatic 10, which I repaired by extracting the win32 rename-refusal fallback into its own function rather than by moving a baseline.

[thymikee]

At 9ccc2bc the inode and claim-token checks stop the race from the last review, and the five release sites you changed now keep the task's own error. Two gaps remain.

The reclaim still has a window where the lock path is empty while a live lock sits under the .reclaimed- name. In reclaimProcessLockDirectory, nothing is at the lock path between the renameSync and the rename back. A polling contender can mkdir there. If it has already written its record, the rename back fails with ENOTEMPTY, that error is swallowed, and both processes hold the lock. When the inode matches, the ps liveness probe also runs inside that window, and inode numbers can be reused after the stale directory is removed. No test runs a contender during the rename back.

Four callers still release in finally, so ownerReleaseUnverified can replace the task's error: platform-apple/src/runner/runner-artifact.ts:114, runner-cache.ts:113, managed-allocation/src/store.ts:95 and store-lock.ts:27.

A smaller-design question: could a short reclaim mutex (for example mkdir <lock>.reclaim around stat, read, judge and remove) replace the rename aside, the inode compare and the rename back? Under that mutex only the dead owner's directory can be at the path, so nothing has to be put back. And could one withProcessLock(task) helper in host-kit own the release pattern for all nine callers, so they cannot drift apart?

CI is green and there are no conflicts.

[thymikee]

Pushed e62cdd81bb. Took the reclaim-mutex shape you sketched, and the helper for the callers.

1. The rename window is gone because the rename is gone. A reclaim no longer moves anything: it takes mkdir <lock>.reclaim, re-decides from what is on disk, and removes in place.

• Nobody else is deciding while we hold the mutex, so the path is never emptied out from under a contender and there is no rename back to fail. The mutex keeps the .lock suffix (x.reclaim.lock) so a sibling scanner still reads the name as a lock, and the two-contenders test now asserts nothing matching .reclaim survives either.
• The re-decision compares inode, directory mtime, liveness, and claim token. Mtime earns its place: a recycled inode can answer your reuse objection, and an acquisition that has only managed its mkdir has written nothing yet to identify itself — but the directory it created a moment ago is not the one abandoned past its grace.
• Removal is attributed, not recursive-by-default. A directory whose record was read goes as that claim's property. A directory with no record is only ever unlink(owner.json) + rmdir, so contents nobody attributed to a claim are never destroyed — an abandoned lock with no record and something else inside is left alone.
• releaseProcessLock lost its rmSync({recursive: true}) for the same reason: it unlinks the record it verified and removes the directory only while empty.

The contender you asked for, injected at the moment the judge is admitted:

a contender that claims the path during a reclaim keeps its lock

It clears the dead claim, publishes itself, and the acquirer must time out with ownerLiveness: 'live' on its record, intact, token and all. Drop the re-decision and that one goes red on its own (1 failed | 27 passed) with its record deleted. Three more pin the mutex itself, each with a mutant that only it catches: a mutex another process holds leaves the abandoned lock standing (bypass the mutex → red), a mutex left behind by a dead process is cleared by age, and a rmdir that cannot empty the directory leaves the judged record rather than forcing it. The rename rule now lives in atomic-publish-ownership.test.ts, which used to allow exactly two renames between the lock path and its reclaimed name and now asserts the file contains no renameSync/asidePath/.reclaimed- at all.

2. Four finally callers, and the bug one of them was hiding. All four (runner-artifact.ts:114, runner-cache.ts:113, managed-allocation/src/store.ts:95, store-lock.ts:27) went to the helper, so ownerReleaseUnverified can no longer speak over the work that failed. The three that already hand-rolled the correct precedence (withDeviceClaimLock, withRunnerLeaseLock, setupManagedAgentBrowser) lost that copy too.

setupManagedAgentBrowser had a second defect from the same shape: if (freshStatus.installed) return freshStatus; returned from inside the try, skipping the only await release() in the function, and left the install lock for the stale-clear path to notice five seconds later. managed agent-browser setup gives the lock back on every path out runs setup twice — the second call takes that early return — and asserts no .lock directory survives anywhere under the state dir. Take the release out of the helper's success path and that test both fails and burns its whole 5s budget waiting on the lock it leaked.

3. Not every caller is task-shaped, and one should stay best-effort. runner-device-set.ts hands its release across a request boundary, snapshot-source/host.ts acquires in the background and releases when it is told to; neither has a task to wrap. swift-cache.ts releases best-effort on the success path on purpose: a helper that finished compiling should not fail its caller because a lock file lingered, and withProcessLock's success path is deliberately strict, because a lock this process could not give back is not a completed task.

Gate: pnpm check:affected --run green, 28 process-lock tests, plus eager-closure-budgets and test-file-size-ratchet explicitly. pnpm typecheck also caught one thing worth noting: tsc -p tsconfig.json alone does not see packages/host-kit/** test files, and a leftover RECLAIMED_MARK reference survived until the real typecheck ran.

[thymikee]

Verified at e62cdd81bb, which is the shape you asked for rather than a patch over the window:

• Reclaim runs under a mkdir-based mutex on <lock>.reclaim (RECLAIM_MUTEX_SUFFIX), so the judge/read/remove sequence never leaves the lock path missing for a polling contender. Nothing has to be renamed back, which is what made the old renameSync round-trip racy (ENOTEMPTY swallowed → two holders).
• Ownership is a claim token, not an inode: claimToken: crypto.randomUUID() is published with the owner record and a release that finds a different token gets not-owner and leaves the directory alone. Inode reuse can no longer make a stale release look valid.
• The release pattern is owned by one helper: withProcessLock is exported from @agent-device/host-kit/file and the four sites that were releasing in finally (runner-artifact.ts, runner-cache.ts, managed-allocation/src/store.ts, managed-allocation/src/store-lock.ts) go through it, along with runner-lease.ts, device-claim-store.ts and agent-browser-tool.ts. Callers can no longer forget the release or release someone else's lock.

process-lock.test.ts covers the mutex, the token mismatch and the abandoned-owner reclaim.

[thymikee]

Reviewed at e62cdd8. Reclaim now runs under the .reclaim mutex and keeps the lock path in place, so the empty-path window and the swallowed ENOTEMPTY from the last review are gone, and a reclaimer that crashes does not wedge contenders. A few gaps remain.

A failed runner build can now show a lock error instead of the build error. runner-artifact.ts#L527 still releases in finally, and that release ends in releaseLock() (runner-device-set.ts#L126), which this PR makes throw ownerReleaseUnverified when the owner record cannot be read. When xcodebuild build-for-testing fails and the record is unreadable, the user sees "Cannot verify ownership" instead of the xcodebuild failure and its hint. Could the error path ignore the release error and rethrow the build error, as withProcessLock does, or could this redirect go through withProcessLock?

Reclaim can still remove a lock that a new contender just took. judgedLockIsStillThere at process-lock.ts#L311 returns true when lstat fails, and reclaim then runs a recursive rmSync on the path. Another process can mkdir and publish its record between the two calls. If the path is already gone under the mutex, could reclaim return without touching disk, so the loop retries mkdir?

A similar question at process-lock.ts#L273: for an abandoned directory with no record, clearLockDirectory unlinks owner.json before rmdir. Could a slow acquirer publish its record between the "unwritten" re-check and the unlink? If so, could this path use rmdirSync only and treat ENOTEMPTY as a new owner?

store-filesystem.ts#L117 still says a stale reclaim renames a lock aside, and store.test.ts:131 still builds a .reclaimed-4242-0.lock fixture, but nothing renames a lock aside now; the sibling is <stem>.reclaim.lock. Could the comment and the fixture use the current name?

The two-contender test at process-lock.test.ts#L269 runs both acquirers in one process, and reclaim is synchronous, so the mutex is never contended; with ownerGraceMs: 0, a held mutex would also count as abandoned at once. Could the test hold the mutex while the second contender reclaims, with a nonzero grace?

Would a smaller shape work under the mutex? A directory with a record could be judged by its claim alone, and an empty one by rmdir alone, which would drop the ino/dev/mtime comparison and most of the JudgedLock stats. runner-device-set could also release through withProcessLock, so no caller keeps a hand-written finally.

CI is green on e62cdd8, and there are no conflicts. The next step is the build-error masking in runner-artifact.ts and the reclaim that removes a path after lstat found it absent.

[thymikee]

Addressed at 0f37589085.

A failed build reports the build. buildRunnerXctestrun runs inside withXcodebuildSimulatorSetRedirect, which hands the release to withProcessLock, so the precedence lives in one place: the task's error wins, and an unverifiable release is reported only when the task succeeded. The two sites that are not tasks — the launch-failure release in runner-session.ts and the teardown release in runner-disposal.ts — call releaseXcodebuildSimulatorSetRedirectBestEffort, so no caller keeps a hand-written finally. runner-device-set.test.ts pins both directions: a build that failed outranks the redirect it could not give back, and a build that succeeded still reports the redirect it could not give back.

A reclaim that finds the path gone touches nothing. judgedLockIsStillThere is deleted. clearStaleProcessLock answers true when stat fails, so the loop's next mkdir simply wins the path. Inside the mutex each branch decides again from what is on disk: the dead-claim branch re-reads owner.json and removes only while that record still answers to the judged claim token.

An abandoned directory with no record is rmdir'd and nothing else. removeAbandonedEmptyLock stats, re-asks the age, and calls rmdirSync; ENOTEMPTY is a publication saying so. releaseProcessLock lost its recursive removal for the same reason — unlink the record it verified, rmdir while empty.

Names. store-filesystem.ts and the store.test.ts fixture both say <stem>.reclaim.lock, which is the mutex. The parked-directory name is gone from the code and the docs.

The two-contender test asks with a second of grace now: with zero, the mutex the winner holds reads as abandoned and the test measures a reclaim that skipped the mutex. Where the mutex is held (default 5s grace) the test also asserts the contender kept polling rather than giving up after one attempt. Reclaim is synchronous in-process, so genuine two-process contention is not producible there; the interleaving is injected in a contender that claims the path during a reclaim keeps its lock.

Smaller shape taken. sameDirectory, judgedLockIsStillThere, forceRemoveLockDirectory, unlinkStrayLockPath and readClaimToken are gone with the ino/dev/mtime plumbing; JudgedLock is a three-case union (dead-claim / empty / stray) and the module is 443 lines where it was 464. A stray path is now simply unlinked: the lstat isDirectory guard could not be made red by any mutation, because unlink answers EISDIR for the case it guarded.

What each remaining decision is worth, by mutation:

mutation	test that goes red
withProcessLock rethrows the release error on the task's error path	a build that failed outranks the redirect it could not give back
removeDeadClaimLock removes without comparing the claim token	a contender that claims the path during a reclaim keeps its lock
removeAbandonedEmptyLock swaps rmdirSync for rmSync(recursive)	an abandoned lock with no record and something else inside is left alone
removeAbandonedEmptyLock stops re-asking the age	a lock directory made anew while a reclaim holds the mutex is not the one that was abandoned
releaseXcodebuildSimulatorSetRedirectBestEffort propagates	a redirect handed back after its task keeps quiet about a release it cannot verify

Two reclaim tests are new (a claim published while a reclaim holds the mutex outlives the empty directory it filled, a lock directory made anew…) alongside the three redirect tests. pnpm check:affected --run passes on this tree, as do typecheck, lint, check:layering and check:fallow.

[thymikee]

Reviewed at 0f37589, as a follow-up to the review at e62cdd8. The claim token and the rmdir-only clear under the mutex are in, and the build error now wins over a release error because the precedence lives in withProcessLock. Two gaps remain.

From the code, a release that fails for a reason other than a lost race can wedge the device-set lock until the daemon restarts. Release returns "unverified" on any non-ENOENT read or unlink error, for example EMFILE or EACCES (process-lock.ts#L40, with the call sites at L143, L162 and L168). The lock directory stays, and its record still names this live daemon with a token that nothing holds. releaseXcodebuildSimulatorSetRedirectBestEffort has already set released = true (runner-device-set.ts#L164) and drops the handle, so nothing retries. The next acquire, also from this daemon, sees a live owner in clearStaleProcessLock and never reclaims. Every later runner launch or build then waits 30 s and fails with "Timed out waiting for ...". The comments at process-lock.ts:38-40 and runner-device-set.ts:52-53, 67-68 and 134-135 say this case heals itself. Could a record that names this live process, with a claim token this process no longer holds, count as a dead claim (for example, a module-level set of live tokens that clearStaleProcessLock checks)? Or could the handle stay retryable until releaseLock succeeds? In both cases, a swallowed release should emit a diagnostic.

The bare catch {} in the best-effort helper (runner-device-set.ts#L70) also swallows errors from reconcileXcodebuildSimulatorSetRedirect (L166-170), which restores the host's real ~/Library/Developer/XCTestDevices from its backup. Before this change, runner-disposal.ts and runner-session.ts let those errors propagate. Now a failed restore at teardown is silent, and the symlink can stay pointed at the agent-device set. Could the catch take only an AppError with details.ownerReleaseUnverified === true, rethrow the rest, and call emitDiagnostic on the swallowed path? A test where reconcile throws would show that teardown reports it.

Is the separate best-effort helper still needed? It reads as a second copy of the withProcessLock catch branch, with a wider swallow. Teardown and the launch-failure path could call one handle method that swallows only ownerReleaseUnverified and logs it. The recovery rule for the first gap decides what that method must do, so that rule comes first.

All 18 checks pass at 0f37589, and there are no conflicts. The next step is a recovery rule for a lock with a spent claim token, plus the narrower catch.

[thymikee]

Reviewed at bb64eac, as a follow-up to the review at 0f37589. The delta takes the smaller design the last review asked for: one module-level set and a handle method instead of the standalone helper.

One problem is left in release(). It calls reconcileXcodebuildSimulatorSetRedirect inside try and await releaseLock() inside finally (runner-device-set.ts#L151). If the restore throws (for example EACCES on renameSync) and releaseLock also throws the ownerReleaseUnverified error, the finally error replaces the restore error, and releaseBestEffort (lines 163-168) then swallows it. EACCES and EMFILE often hit both steps together. The user impact: a failed restore of ~/Library/Developer/XCTestDevices goes silent again, and the symlink stays pointed at our simulator set. The new test covers only a restore failure with a successful lock release. Can you catch the reconcile error, still run releaseLock, and rethrow the reconcile error (attaching an unverified release as a cause or a log line)? A test where both the rename and the owner.json unlink fail, asserting that releaseBestEffort rejects with EACCES, would pin it.

A question on the live-claim set: it is per module instance. If process-lock.ts loads twice in one process (a duplicated bundle chunk), could one copy read the other copy's live claim with the same pid as spent and take over a live owner?

Coverage fails only on scripts/__tests__/test-file-size-ratchet.test.ts: runner-session.test.ts is 1961 lines against 1955 at the merge-base. This PR edits that file, so this failure belongs to the PR; a shared redirect mock factory could bring it back under the limit. Typecheck & Package fails with TS2353 on installArgs in packages/platform-android/src/__tests__/snapshot-helper-install.test.ts. This PR does not touch that file, and main fails the same way at 2cafab3 because #2618 removed the field while that test still sets it, so the failure is unrelated to this PR. A rebase will not clear it until main is fixed.

Next: fix the error precedence in release() and the test-file size.

[thymikee]

Reviewed again at 0f3758; answered at 4f18834125, rebased on origin/main at 5f6481cafb.

A release that fails for a reason other than a race no longer wedges the lock, and the reason it could not is now recorded. You were right that the fix had to be about the claim and not the removal. A token is issued per acquisition and held in liveClaimTokens; the release drops it from that set the moment it is asked for, before it touches the filesystem, because from that instant nobody in this process is acting on the claim whatever the unlink does next. clearStaleProcessLock now treats a record naming this live pid under a token this process no longer holds as the dead claim it is, alongside the ones it already reclaimed. So an EACCES, an EMFILE, or an unreadable record costs one diagnostic line and no longer a 30 s wait that only a daemon restart ends. The diagnostic is process_lock_release_unverified at the owning site — inside the release closure, so it covers every lock in the package rather than whichever caller happens to be listening.

The swallow is narrowed and it moved. release() does two things: restore the host's own ~/Library/Developer/XCTestDevices, and give the lock back. Only the second is a teardown's own business — that claim is spent, and the reclaim above reads it as dead — so releaseBestEffort drops exactly an AppError whose details carry ownerReleaseUnverified and rethrows everything else. A device-set restore that answered EACCES now reaches the caller instead of disappearing, which is the shape you asked to see: the test spies renameSync throwing EACCES and asserts the rethrow.

No separate helper. The standalone best-effort function is gone; the handle carries release for the build path, where withProcessLock owns which of two failures gets reported, and releaseBestEffort for the two sites whose own outcome is already decided. One place decides what a teardown forgives.

The double honesty point from the same round was load-bearing, not cosmetic: two fixtures that pretended to be a rival process were naming process.pid with a token it never issued, which after this change is exactly the signature of a spent claim. They name process.ppid now — a live other process, which is what a contender has to be for the test to mean anything.

Mutations, each run against the tree with only that change:

Mutation	Result
drop the spent-claim disjunct from the reclaim decision	the second acquire from this process waits out its whole timeout, the reported failure verbatim
let releaseBestEffort swallow every error, as the first draft did	the EACCES restore test stops reporting it
use the strict release at teardown	the same restore failure displaces the command's own outcome

Gate at 4f18834125: pnpm check:affected --run passed, 619 test files, 4630 tests, typecheck, lint, layering and the Fallow audit clean. The Coverage job had failed at the earlier head on test-file-size-ratchet, which is not inside check:affected's selection: the two per-file redirect doubles grew runner-session.test.ts past its 1,955-line merge-base count. The handle now lives once in runner-session-fixtures.ts, which the family already shares, and the family is five lines shorter; the ratchet and the eager-closure budgets (614 assertions) pass locally.

Honest gaps. The wedge needs a unlink that is refused, which is fault injection at the seam — no device run produces EACCES on its own, so the live coverage is the CI lanes that build and launch the runner through the redirect this PR adds, not a natural occurrence of the fault. The reclaim after a failed release is pinned by an in-process double rather than a second daemon. And there is no win32 evidence here: the forced-removal fallback has no CI.

[thymikee]

Reviewed at 4f18834, as a follow-up to the review at bb64eac. The delta adjusts the spent-claim commit and adds one shared device-set redirect double, but the redirect give-back problem is still open.

In runner-device-set.ts#L149, release() runs reconcileXcodebuildSimulatorSetRedirect in the try and await releaseLock() in finally. If renameSync throws EACCES and the owner.json read or unlink also fails, the finally throws ownerReleaseUnverified and replaces the EACCES; releaseBestEffort (L155-L164) then matches isOwnerReleaseUnverified and drops it. Both steps use the same filesystem, so they tend to fail together. A failed restore of ~/Library/Developer/XCTestDevices then goes silent and the symlink stays on the agent-device simulator set. The restore error should win: catch it, still give the lock back and log an unverified release, then rethrow. A test where both the rename and the owner.json unlink fail, and releaseBestEffort rejects with EACCES, would pin this.

The no-redirect branch looks like it has a related gap. At runner-device-set.ts#L109, await releaseLock() is inside the try, so an unverified release goes to the catch, which reconciles, releases again, and throws COMMAND_FAILED "Failed to redirect XCTest device set path". Would a simulator whose set already is XCTestDevices now fail its build or launch with that misleading redirect error when owner.json is unreadable? Can this branch follow the same rule as teardown?

Could the redirect handle have one ordered give-back instead: restore, then release, where the restore error always wins and an unverified release is only logged? Then release and releaseBestEffort would differ only in whether an unverified release throws, and the three exit paths of acquireXcodebuildSimulatorSetRedirect (L109, L129, L149) would share it. What needs to be decided first is where that precedence rule lives, in withProcessLock or in the handle.

Two doc fixes. The JSDoc at runner-session-fixtures.ts#L189 has a garbled sentence ("the the host's device set came back, and which door..."). The PR body says "Four files; one is outside host-kit", but the diff touches 30 files: device-claim-store, the managed-allocation store, agent-browser setup, and the Apple runner artifact, lease and disposal paths move onto withProcessLock, which changes which error those callers report. Can the body and the CHANGELOG entry describe that migration and the new error precedence? The bb64eac question about a second bundled copy of process-lock.ts treating another copy's live claim as spent is also still open.

All checks passed at 4f18834 and there are no conflicts. The newer head d055691 is the same patch rebased onto main, so this applies to it unchanged. Next: make the restore error win in release() and add the double-failure test.

[thymikee]

Rebased on origin/main at e3cbc91a1a; head is f334710c9b. No textual conflict, and the branch met main's new close-finalization tests in a way worth naming.

Those tests caught a real gap between the two branches. runner-close-finalization.test.ts and runner-session-close.test.ts landed on main answering the device-set redirect with { release }, which was the whole handle the day they were written; disposal in this branch asks the session for releaseBestEffort. Every test in those two files that closes a session with a redirect failed with session.simulatorSetRedirect?.releaseBestEffort is not a function — four tests, which is how the local gate found it before CI did.

The double is what is incomplete, not the teardown, so f334710c9b takes the handle from runner-session-fixtures.ts in both files, the module their six sibling test files already use for it, and drops the per-file spy that appeared only in setup. Teardown is not guarded for a handle that does not implement its own type: that would teach the next caller that handing one over is allowed.

Why typecheck passed over it: mockAcquireXcodebuildSimulatorSetRedirect is a bare vi.fn(), so mockResolvedValue accepts any shape. The shared double is what makes the seven session-family doubles agree now; making that mock answer-typed is a separate change and this PR does not do it. Proof of coverage: no { release }-only literal remains anywhere except the object runner-device-set.ts builds and the session field's own two-operation type.

Gate at f334710c9b: pnpm check:affected --run passed, 621 test files, 4644 tests, typecheck, lint, layering and the Fallow audit clean; the apple-runner lane alone is 44 files, 430 tests, including the four that were red on the unmerged pair. Earlier evidence at 4f18834125 and 0f37589 stands as attributed there.

[thymikee]

Checked at f334710. The new commit only moves the two close tests onto the shared redirect double, and runner-device-set.ts is unchanged, so the review at 4f18834 still applies.

In release() at runner-device-set.ts#L144-L151, an unverified lock release thrown from finally still replaces a restore error, and releaseBestEffort then drops it. A failed restore of ~/Library/Developer/XCTestDevices stays silent. The no-redirect branch at L109-L111 still turns an unverified release into "Failed to redirect XCTest device set path".

All checks pass and there are no conflicts. Next: make the restore error win in one ordered give-back, and add the test where both the rename and the owner.json unlink fail.

[thymikee]

Reviewed at 5680258423 and reaffirmed at 5680920139; answered at 868d0ad713.

The restore error now wins, and there is one ordered give-back. release() reconciled inside try and gave the lock back in finally, so when both failed the finally threw ownerReleaseUnverified over the restore's EACCES, and releaseBestEffort then dropped it exactly as you described: a XCTestDevices left pointed at this simulator's set and the only report naming the lock. The handle now runs restore, then release; the restore failure is thrown whichever door was used, the lock goes back regardless, and the release's own failure is recorded either way. release and releaseBestEffort differ in the one thing you said they should: whether the caller also throws an unverified release.

Where the precedence rule lives: the handle, not withProcessLock. That helper is host-kit and knows only that a lock was taken and a task ran; which of "the host's device set could not be restored" and "this claim could not be verified" a caller should hear is a fact about this redirect, and putting it in the lock would make every other lock caller answer a question it does not have. withProcessLock keeps its own, narrower rule — the task outranks the give-back — which is what the artifact, lease, store and publish callers needed.

The no-redirect branch followed the same rule. It ran releaseLock() inside the try, so an unverified ownership check landed in the catch, reconciled, released again, and raised the redirect error for a redirect that was never needed. A test reaches that window by answering the lock at the seam the module acquires it through (vi.mock of ../host.ts, passing everything else through) and asserts a null handle; putting the release back inside the try brings Failed to redirect XCTest device set path back verbatim, which the run printed. The third exit path — a redirect whose install failed — now also survives a second failure in its own clean-up: the log records the restore that could not run and the caller still hears why the redirect failed.

The new tests. Both faults at once (restore renameSync refusing EACCES, record refusing the unlink) rejected through release and releaseBestEffort alike with EACCES, the lock still standing as proof the pair happened. Reverting the order to the old finally reddens that test through both doors. The install-failure test keeps EPERM visible inside the redirect error, and the no-redirect test asserts the null handle.

The bundled-copy question from bb64eac was real and is now answered in code, not prose. A pid names a process, and the spent-claim rule reads a record that names one; two loadings of process-lock.ts in one process share the pid and the start time and cannot see each other's liveClaimTokens, so the second one's live claim read as spent. Records carry claimIssuerId now and the rule requires it to be this loading's; a record without one — written before issuers, or by other code — is not evidence of a spent claim and stays subject to the liveness answer, so nothing that reclaimed before stops reclaiming now. The field is parsed as optional rather than added to the required shape, because a pre-upgrade record must still parse instead of reading as unreadable. Deleting the issuer check reddens the new test alone, 1 failed 31 passed.

Doc fixes. The fixtures comment lost a clause mid-edit and now says what the double actually does. The body overstated the diff as four files; it says 32 and names the migration, and the CHANGELOG gained the entries it was missing: the withProcessLock error precedence and the redirect's ordered give-back, beside the lock recovery it already described.

Gate at 868d0ad713: pnpm check:affected --run passed, 622 test files, 4648 tests, typecheck, lint, layering and Fallow clean.

[thymikee]

Reviewed at 868d0ad, as a follow-up to the review at f334710. The handle's give-back and the no-redirect branch now let a restore error win, but the install-failure catch still hides one.

In acquireXcodebuildSimulatorSetRedirect (runner-device-set.ts#L137), take an install that fails after renameSync(xctestDeviceSetPath, backupPath), for example symlinkSync with EPERM, where the rename back also fails with EACCES. The catch throws with the EPERM text only, and the restore error reaches only the ios_runner_xctest_device_set_restore_failed diagnostic. The host's XCTestDevices then stays in the backup path until the next acquire, and the CLI and MCP output do not say so. The new install-failure test asserts only EPERM, so it pins this gap. Can this catch add restoreError to the AppError details, with a hint that names backupPath, and can the test assert that details.restoreError matches /EACCES/?

Would one ordered give-back helper do for all three sites? It would restore, then release, and return { restoreFailure, releaseFailure }, so the install catch uses the same result as the handle instead of its own try/catch. Then this gap cannot come back. If there is a reason to keep the sites separate, can the PR say it?

All 18 checks pass and there are no conflicts. Next: surface the restore error from the install catch, with its test.