fix(host-kit): a reclaim decides in place, and one helper owns giving the lock back

Rename-aside made a reclaim a two-step transaction: move the abandoned directory out from under
the lock path, judge it there, and move it back if it turned out to belong to somebody else.
Between those steps the lock path is simply absent, and a contender polling the path reads that
as free: it claims the path and publishes, and the rename back returns `ENOTEMPTY` into a
`catch {}`. The judged directory ends up nobody's, and the one check meant to notice the swap —
inode and device — can be answered by a recycled inode with a recycled token.

Removal happens in place now, behind `mkdir <lock>.reclaim`:

- nobody else is judging while we are, so the path is never emptied out from under a contender;
- the judgement is re-made from what is on disk before anything is removed: same inode, same
  directory mtime, no live owner inside, and the record either carrying the judged claim token or
  still absent. A directory created a moment ago cannot answer for one abandoned past its grace;
- a directory whose record was read goes as that claim's property. One with no record is emptied
  and `rmdir`'d, so contents nobody attributed to a claim are never destroyed.

`releaseProcessLock` lost its recursive removal for the same reason: it unlinks the record it
verified and removes the directory only while empty.

The callers were each answering "which of two failures do I report?" by hand, and four answered
with `finally`: `runner-artifact.ts`, `runner-cache.ts`, `managed-allocation/src/store.ts` and
`store-lock.ts`. There an unverified release spoke over the task's own failure with
`ownerReleaseUnverified`, which describes a lock and not the work. All four now call
`withProcessLock({ acquire, task })`, which releases best-effort when the task failed and
strictly when it did not. `managed agent-browser setup gives the lock back on every path out`
pins a second bug that shape was hiding: setup returned early when the package was already
installed, skipping the only `await release()` in the function and leaving the lock for the
stale-clear path to notice five seconds later.

`runner-device-set.ts` hands its release across a request boundary and `snapshot-source/host.ts`
acquires in the background, so neither is a task-shaped caller. `swift-cache.ts` keeps swallowing
a release error on success on purpose: a compiled helper should not fail because a lock lingered.

Co-authored-by: Apex by Callstack <noreply@callstack.com>

Author: thymikee

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';

packages/host-kit/src/internal/process-lock.test.ts

@@ -11,12 +11,15 @@ vi.mock('./host-process.ts', async (importOriginal) => {
   return { ...actual, isProcessZombie: (pid: number) => zombiePids.has(pid) };
 });
 
-import { acquireProcessLock, type ProcessLockOwner } from './process-lock.ts';
+import {
+  acquireProcessLock,
+  withProcessLock,
+  type ProcessLockOwner,
+  type ProcessLockRelease,
+} from './process-lock.ts';
 import { readProcessStartTime } from './host-process.ts';
 import { mkdtempForTestSync } from './tmp-dir.fixtures.ts';
 
-const RECLAIMED_MARK = '.reclaimed-';
-
 let tmpDir: string;
 
 beforeEach(() => {
@@ -293,13 +296,14 @@ test('one abandoned lock offered to two contenders is held by exactly one of the
   assert.ok(reason instanceof AppError);
   assert.equal(reason.details?.ownerLiveness, 'live');
   await (acquired[0] as PromiseFulfilledResult<() => Promise<void>>).value();
-  assert.deepEqual(listReclaimedSiblings(tmpDir), []);
+  assert.deepEqual(listReclaimSiblings(tmpDir), []);
 });
 
-function listReclaimedSiblings(directory: string): string[] {
+/** A reclaim that finishes leaves neither a parked directory nor a mutex behind. */
+function listReclaimSiblings(directory: string): string[] {
   return fs
     .readdirSync(directory)
-    .filter((entry) => entry.includes(RECLAIMED_MARK))
+    .filter((entry) => entry.includes('.reclaim'))
     .sort();
 }
 
@@ -374,28 +378,39 @@ test('acquireProcessLock reclaims a stray path in place of the lock directory',
   assert.equal(fs.existsSync(lockDirPath), false);
 });
 
-test('a forced reclaim leaves a directory that a live owner republished', async () => {
-  const lockDirPath = path.join(tmpDir, 'republished.lock');
+test('a contender that claims the path during a reclaim keeps its lock', async () => {
+  const lockDirPath = path.join(tmpDir, 'claimed-during-reclaim.lock');
+  const mutexPath = path.join(tmpDir, 'claimed-during-reclaim.reclaim.lock');
   const ownerFilePath = path.join(lockDirPath, 'owner.json');
   fs.mkdirSync(lockDirPath);
   fs.writeFileSync(
     ownerFilePath,
     JSON.stringify({ pid: 999_999_999, startTime: null, acquiredAtMs: Date.now() }),
   );
+  stampDirectoryAbandoned(lockDirPath);
 
-  // A win32 handle refuses the rename, and by the time the forced removal would run a
-  // live holder has claimed the path: the forced removal must not reach its directory.
-  const realRename = fs.renameSync;
-  const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation(((
-    from: fs.PathLike,
-    to: fs.PathLike,
+  // The moment a contender is admitted to judging this lock, another process clears the dead
+  // claim and publishes itself. The judge must find a different directory, not a directory to
+  // remove: nothing here stands between the two states, because nothing is moved out of the
+  // way first.
+  let claimed = false;
+  const realMkdir = fs.mkdirSync;
+  const mkdirSpy = vi.spyOn(fs, 'mkdirSync').mockImplementation(((
+    target: fs.PathLike,
+    options?: fs.MakeDirectoryOptions & { recursive: true },
   ) => {
-    if (!String(to).includes(RECLAIMED_MARK)) return realRename(from, to);
-    fs.rmSync(String(from), { recursive: true, force: true });
-    fs.mkdirSync(String(from));
-    fs.writeFileSync(path.join(String(from), 'owner.json'), JSON.stringify(currentProcessOwner()));
-    throw Object.assign(new Error('operation not permitted'), { code: 'EPERM' });
-  }) as typeof fs.renameSync);
+    if (String(target) !== mutexPath || claimed) {
+      return realMkdir(target as string, options as fs.MakeDirectoryOptions);
+    }
+    claimed = true;
+    fs.rmSync(lockDirPath, { recursive: true, force: true });
+    fs.mkdirSync(lockDirPath);
+    fs.writeFileSync(
+      ownerFilePath,
+      JSON.stringify({ ...currentProcessOwner(), claimToken: 'contender-claim' }),
+    );
+    return realMkdir(target as string, options as fs.MakeDirectoryOptions);
+  }) as typeof fs.mkdirSync);
 
   try {
     await assert.rejects(
@@ -409,48 +424,76 @@ test('a forced reclaim leaves a directory that a live owner republished', async
       (error: unknown) => {
         assert.ok(error instanceof AppError);
         assert.equal(error.details?.ownerLiveness, 'live');
+        assert.equal(error.details?.ownerPid, process.pid);
         return true;
       },
     );
-    assert.equal(fs.existsSync(ownerFilePath), true);
+    assert.equal(claimed, true);
+    const record = JSON.parse(fs.readFileSync(ownerFilePath, 'utf8')) as {
+      pid: number;
+      claimToken: string;
+    };
+    assert.equal(record.pid, process.pid);
+    assert.equal(record.claimToken, 'contender-claim');
   } finally {
-    renameSpy.mockRestore();
+    mkdirSpy.mockRestore();
   }
 });
 
-test('a reclaim that cannot remove the directory it moved aside still holds the lock', async () => {
-  const lockDirPath = path.join(tmpDir, 'immovable.lock');
+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.
+  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;
+    },
+  );
+  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 realRemove = fs.rmSync;
-  const removeSpy = vi.spyOn(fs, 'rmSync').mockImplementation(((target: fs.PathLike, options) => {
-    if (String(target).includes(RECLAIMED_MARK)) {
-      throw Object.assign(new Error('directory is busy'), { code: 'EBUSY' });
-    }
-    return realRemove(target, options);
-  }) as typeof fs.rmSync);
+  const release = await acquireProcessLock({
+    lockDirPath,
+    owner: currentProcessOwner(),
+    timeoutMs: 500,
+    pollMs: 1,
+  });
 
-  try {
-    const release = await acquireProcessLock({
-      lockDirPath,
-      owner: currentProcessOwner(),
-      timeoutMs: 500,
-      pollMs: 1,
-    });
-    assert.equal(fs.existsSync(path.join(lockDirPath, 'owner.json')), true);
-    await release();
-  } finally {
-    removeSpy.mockRestore();
-  }
+  assert.equal(fs.existsSync(path.join(lockDirPath, 'owner.json')), true);
+  assert.equal(fs.existsSync(mutexPath), false);
+  await release();
 });
 
-test('a live owner published between the stale read and the rename keeps its lock', async () => {
-  const lockDirPath = path.join(tmpDir, 'stolen-race.lock');
+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(
@@ -459,93 +502,123 @@ test('a live owner published between the stale read and the rename keeps its loc
   );
   stampDirectoryAbandoned(lockDirPath);
 
-  // The dead record is read, and before the rename lands another contender reclaims the
-  // path, publishes itself, and goes live. The rename then moves that live directory, and
-  // the only thing that can tell it apart from the one judged abandoned is the directory
-  // itself.
-  let republished = false;
-  const realRename = fs.renameSync;
-  const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation(((
-    from: fs.PathLike,
-    to: fs.PathLike,
-  ) => {
-    if (!String(to).includes(RECLAIMED_MARK) || republished) return realRename(from, to);
-    republished = true;
-    fs.rmSync(String(from), { recursive: true, force: true });
-    fs.mkdirSync(String(from));
-    fs.writeFileSync(path.join(String(from), 'owner.json'), JSON.stringify(currentProcessOwner()));
-    return realRename(from, to);
-  }) as typeof fs.renameSync);
+  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: { pid: 999_999_998, startTime: null, acquiredAtMs: Date.now() },
+          owner: currentProcessOwner(),
           timeoutMs: 50,
           pollMs: 1,
         }),
       (error: unknown) => {
         assert.ok(error instanceof AppError);
-        assert.equal(error.details?.ownerLiveness, 'live');
-        assert.equal(error.details?.ownerPid, process.pid);
+        assert.equal(error.details?.ownerPid, 999_999_999);
         return true;
       },
     );
-    assert.equal(republished, true);
-    assert.equal(
-      (JSON.parse(fs.readFileSync(ownerFilePath, 'utf8')) as { pid: number }).pid,
-      process.pid,
-    );
-    assert.deepEqual(
-      fs
-        .readdirSync(tmpDir)
-        .filter((name) => name.includes(RECLAIMED_MARK))
-        .sort(),
-      [],
-    );
+    assert.equal(fs.existsSync(ownerFilePath), true);
   } finally {
-    renameSpy.mockRestore();
+    removeSpy.mockRestore();
   }
 });
 
-test('a reclaim whose directory another contender moved aside retries and acquires', async () => {
-  const lockDirPath = path.join(tmpDir, 'lost-race.lock');
+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(
-    path.join(lockDirPath, 'owner.json'),
-    JSON.stringify({ pid: 999_999_999, startTime: null, acquiredAtMs: Date.now() }),
-  );
+  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));
 
-  // The contender that loses the rename finds the path already gone and cannot have
-  // cleared anything; it goes back to `mkdir`, which is what decides the lock.
-  let attempted = 0;
-  const realRename = fs.renameSync;
-  const renameSpy = vi.spyOn(fs, 'renameSync').mockImplementation(((
-    from: fs.PathLike,
-    to: fs.PathLike,
-  ) => {
-    if (!String(to).includes(RECLAIMED_MARK)) return realRename(from, to);
-    attempted += 1;
-    fs.rmSync(lockDirPath, { recursive: true, force: true });
-    throw Object.assign(new Error('no such directory'), { code: 'ENOENT' });
-  }) as typeof fs.renameSync);
+  // 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);
+});
 
-  try {
-    const release = await acquireProcessLock({
-      lockDirPath,
-      owner: currentProcessOwner(),
-      timeoutMs: 500,
-      pollMs: 1,
-    });
-    assert.equal(attempted, 1);
-    assert.equal(fs.existsSync(path.join(lockDirPath, 'owner.json')), true);
-    await release();
-  } finally {
-    renameSpy.mockRestore();
-  }
+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 {