fix(runner): stop a displaced ACPX recovery lock holder's release from deleting a replacement lock

A recovery lock holder's own release used a plain, unconditional unlink.
A stale-lock break can judge a still-live holder's lock stale and remove
it while that holder is still mid-recovery; a later admission can then
win a fresh lock at the same path before the first holder finishes. The
first holder's own release then deleted that live replacement, letting a
third admission win its own lock and run recovery concurrently with the
second.

Fix release to pin its own lock instance under a private path for as
long as it holds the lock, then release by that pinned identity: a match
proves it caught its own lock, safe to delete; a mismatch means a
replacement was caught by mistake, so this restores it instead.

Co-authored-by: Paperclip <noreply@paperclip.ing>

packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.test.ts

@@ -1464,11 +1464,11 @@ describe("ACPX runtime sandbox", () => {
 
     // The second admission's own fresh recovery lock is now live at the
     // shared path, created only after it broke the crashed recoverer's
-    // stale one for real. A real lock carries no content — like the lease
-    // files beside it, only its presence and its age ever matter — so its
-    // identity, not its content, is what proves whether it survives
-    // untouched: an admission that deletes it and creates its own new one
-    // in its place would look identical by content alone.
+    // stale one for real. This check asserts on the lock's identity, not
+    // its content: what follows never deletes and recreates this exact
+    // lock, only moves it aside and links it straight back, so its device
+    // and inode stay stable throughout and prove whether it survives
+    // untouched.
     const freshLockIdentity = await waitForAcpxSandboxGateState(async () => {
       const contents = await readFile(recoveryLockPath, "utf8").catch(
         () => null,
@@ -1516,6 +1516,105 @@ describe("ACPX runtime sandbox", () => {
     await secondProcess.removeOwnedAcpxRuntimeSandboxRoot(secondSandbox);
     await firstProcess.removeOwnedAcpxRuntimeSandboxRoot(firstSandbox);
   });
+
+  it("never removes a replacement recovery lock after a live recovery's own lock is wrongly broken as stale", async () => {
+    const fixture = await sandboxFixture("codex");
+    const probe = await prepareAcpxRuntimeSandbox({
+      binding: fixture.binding,
+      agent: "codex",
+    });
+    await releaseAcpxRuntimeSandboxRootClaim(probe);
+    const gatePath = join(dirname(probe.root), `${basename(probe.root)}.gate`);
+    const recoveryLockPath = `${gatePath}.recovering`;
+
+    // A holder crashed while it held the gate: write a gate file naming a
+    // process that has already exited.
+    const deadHolder = spawnSync(process.execPath, ["-e", "0"]);
+    const deadHolderContents = String(deadHolder.pid);
+    await writeFile(gatePath, deadHolderContents, { flag: "wx" });
+
+    vi.resetModules();
+    const firstProcess: typeof import("./runtime-sandbox.js") =
+      await import("./runtime-sandbox.js");
+    let releaseFirstLock: () => void = () => {};
+    const firstLockPaused = new Promise<void>((settle) => {
+      releaseFirstLock = settle;
+    });
+    const firstRecovered = firstProcess.prepareAcpxRuntimeSandbox(
+      { binding: fixture.binding, agent: "codex" },
+      { afterRecoveryLockAcquired: () => firstLockPaused },
+    );
+
+    // Wait until the first admission has won a genuinely live recovery
+    // lock, before its own recovery pass ever runs.
+    await waitForAcpxSandboxGateState(async () => {
+      await expect(stat(recoveryLockPath)).resolves.toBeDefined();
+    });
+
+    // A second admission finds the same dead gate and the first admission's
+    // lock in its way. Move the clock forward past the recovery lock's own
+    // stale threshold before the second admission judges the lock's age, so
+    // it wrongly treats the first admission's still-live lock as abandoned
+    // by a crashed recoverer — the first admission has done nothing wrong;
+    // the lock is simply old enough by the mocked clock to look that way.
+    const dateNowSpy = vi
+      .spyOn(Date, "now")
+      .mockReturnValue(Date.now() + 5_500);
+    vi.resetModules();
+    const secondProcess: typeof import("./runtime-sandbox.js") =
+      await import("./runtime-sandbox.js");
+    let releaseSecondLock: () => void = () => {};
+    const secondLockPaused = new Promise<void>((settle) => {
+      releaseSecondLock = settle;
+    });
+    // The second admission's first attempt at the lock is guaranteed to hit
+    // the first admission's still-present lock and go through a break
+    // attempt, since the first admission never releases before this signal
+    // fires — so reaching this seam again proves the second admission's own
+    // fresh acquisition, on its next retry, not a first-attempt success.
+    let markSecondLockAcquired: () => void = () => {};
+    const secondLockAcquired = new Promise<void>((settle) => {
+      markSecondLockAcquired = settle;
+    });
+    const secondRecovered = secondProcess.prepareAcpxRuntimeSandbox(
+      { binding: fixture.binding, agent: "codex" },
+      {
+        afterRecoveryLockAcquired: () => {
+          markSecondLockAcquired();
+          return secondLockPaused;
+        },
+      },
+    );
+
+    // The second admission breaks the first admission's real lock (it looks
+    // stale under the mocked clock) and, on its next retry, wins a fresh
+    // lock of its own at the same path — the replacement the first
+    // admission's own eventual release must not remove.
+    await secondLockAcquired;
+    dateNowSpy.mockRestore();
+
+    // The first admission resumes now, unaware its own lock was already
+    // broken. It must never remove the second admission's live replacement
+    // lock when its own recovery finishes and it releases what it believes
+    // is still its own lock. The second admission is still paused and has
+    // not released its own lock itself, so the lock still being present
+    // right after the first admission's own recovery finishes proves the
+    // first admission's release left it alone.
+    releaseFirstLock();
+    const firstSandbox = await firstRecovered;
+    expect(firstSandbox.root).toBe(probe.root);
+    await expect(stat(recoveryLockPath)).resolves.toBeDefined();
+
+    // The second admission now proceeds with its own recovery pass, using
+    // its still-intact replacement lock, and can claim the same, now-freed
+    // root for itself too.
+    releaseSecondLock();
+    const secondSandbox = await secondRecovered;
+    expect(secondSandbox.root).toBe(probe.root);
+
+    await firstProcess.removeOwnedAcpxRuntimeSandboxRoot(firstSandbox);
+    await secondProcess.removeOwnedAcpxRuntimeSandboxRoot(secondSandbox);
+  });
 });
 
 async function sandboxFixture(agent: "pi" | "claude" | "codex") {

packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.ts

@@ -139,6 +139,14 @@ const ACPX_SANDBOX_ROOT_GATE_RECOVERY_LOCK_SUFFIX = ".recovering";
 // later attempt then clears it, instead of leaving every admission for this
 // root to wait out the full gate acquire timeout.
 const ACPX_SANDBOX_ROOT_GATE_RECOVERY_LOCK_STALE_MS = 5_000;
+// Infix for the private path a recovery lock holder pins its own lock
+// instance under for as long as it holds the lock. Distinct from
+// `ACPX_SANDBOX_ROOT_GATE_REAP_INFIX`, whose files are always short-lived
+// (created and removed within a single function call): this pin instead
+// lives for a holder's entire recovery pass, so a test or a directory scan
+// that expects only transient reap files to ever appear must not mistake it
+// for one. See `withAcpxRuntimeSandboxRootGateRecoveryLock`.
+const ACPX_SANDBOX_ROOT_GATE_RECOVERY_LOCK_PIN_INFIX = ".holder-";
 
 export interface AcpxRuntimeSandbox {
   root: string;
@@ -1136,14 +1144,25 @@ async function withAcpxRuntimeSandboxRootGateRecoveryLock(
     );
     return;
   }
-  // The lock's own existence, at this one well-known path, is the whole
-  // exclusivity primitive: nothing here ever inspects its content, only its
-  // presence and its age, so — like the lease files beside it — it carries
-  // no content and is never synced. Losing an un-synced create on a crash
-  // only means the lock looks absent right after a reboot, the same as a
-  // clean release; it never causes a live lock to look absent.
+  // Pin this exact lock instance under a private path this holder alone
+  // controls, for as long as it holds the lock. A plain device-and-inode
+  // check taken now and compared again at release time would not be safe on
+  // its own: if a stale-lock break removes the shared name out from under
+  // this holder in between, the filesystem can reuse the just-freed inode
+  // number for an unrelated replacement lock (observed on this file's own
+  // tmp filesystem, the same hazard the gate's own identity checks already
+  // guard against), so a replacement could carry the very same identity the
+  // removed lock had. This extra link keeps that exact inode allocated for
+  // as long as the pin survives, so its device and inode can never be
+  // confused with a later, unrelated file's — the pin is this holder's own
+  // private name, so no break or release anywhere else in this file ever
+  // touches it, and only this holder's own release removes it.
+  const ownPinPath = `${lockPath}${ACPX_SANDBOX_ROOT_GATE_RECOVERY_LOCK_PIN_INFIX}${process.pid}-${randomBytes(8).toString("hex")}`;
+  let ownEntry: BigIntStats;
   try {
     await handle.chmod(PRIVATE_FILE_MODE);
+    await link(lockPath, ownPinPath);
+    ownEntry = await lstat(ownPinPath, { bigint: true });
   } finally {
     await handle.close();
   }
@@ -1151,7 +1170,92 @@ async function withAcpxRuntimeSandboxRootGateRecoveryLock(
     await dependencies.afterRecoveryLockAcquired?.();
     await recovery();
   } finally {
-    await unlink(lockPath).catch((error) => {
+    await releaseAcpxRuntimeSandboxRootGateRecoveryLock(
+      lockPath,
+      ownPinPath,
+      ownEntry,
+    );
+  }
+}
+
+// Releases this holder's own recovery lock. This must remove only the exact
+// lock instance this holder acquired, never a replacement a later admission
+// has since acquired at the same path — so a plain, unconditional
+// `unlink(lockPath)` is never safe here. `breakStaleAcpxRuntimeSandboxRootGateRecoveryLock`
+// can judge this holder's own still-live lock stale (its own stale threshold
+// is a heuristic, not a proof of death) and remove it while this holder is
+// still genuinely mid-recovery; a later admission can then win a fresh lock
+// of its own at the now-vacant path before this holder's recovery finishes.
+// From this holder's point of view, by the time it releases, `lockPath` can
+// therefore name either its own lock (the common case) or a different
+// admission's live replacement — an unconditional unlink cannot tell these
+// apart, and would delete that replacement right along with its own,
+// letting a third admission win a fresh lock and run its own recovery
+// concurrently with whichever admission the replacement belongs to.
+//
+// This call instead captures whatever currently names `lockPath` with the
+// same atomic `rename` stale-lock recovery uses, then proceeds only by the
+// identity `ownPinPath` still pins: a match proves this call caught its own
+// lock, safe to delete for good; a mismatch means a different admission's
+// live lock was caught by mistake, so this call restores it immediately
+// with `link`, not `rename`, so a further admission that has since taken
+// the now-momentarily-empty shared path is never clobbered by the restore.
+// Either way, this holder's own pin is no longer needed once the shared
+// path is no longer its concern, so this call always releases it last.
+async function releaseAcpxRuntimeSandboxRootGateRecoveryLock(
+  lockPath: string,
+  ownPinPath: string,
+  ownEntry: BigIntStats,
+): Promise<void> {
+  try {
+    const capturePath = `${lockPath}${ACPX_SANDBOX_ROOT_GATE_REAP_INFIX}${process.pid}-${randomBytes(8).toString("hex")}`;
+    try {
+      await rename(lockPath, capturePath);
+    } catch (error) {
+      // Nothing at the shared path to release directly.
+      if (errorCode(error) === "ENOENT") return;
+      throw error;
+    }
+    const capturedEntry = await lstat(capturePath, { bigint: true }).catch(
+      (error) => {
+        if (errorCode(error) === "ENOENT") return null;
+        throw error;
+      },
+    );
+    if (capturedEntry === null) {
+      // The capture is gone already: `capturePath` is a private path only
+      // this call knows, so nothing else should ever remove it, but this
+      // is defensive against that regardless.
+      return;
+    }
+    if (
+      capturedEntry.dev === ownEntry.dev &&
+      capturedEntry.ino === ownEntry.ino
+    ) {
+      await unlink(capturePath).catch((error) => {
+        if (errorCode(error) !== "ENOENT") throw error;
+      });
+      return;
+    }
+    // The rename caught a lock that is not this holder's own: a different
+    // admission's live lock was at the shared path. Put it straight back,
+    // unless a further admission has since taken the now-momentarily-empty
+    // shared path.
+    try {
+      await link(capturePath, lockPath);
+    } catch (error) {
+      if (errorCode(error) !== "EEXIST") throw error;
+      // A further admission grabbed the shared path first: leave the
+      // captured lock under its own private name rather than clobber that
+      // claim. Its real owner's own release finds it later, the same way
+      // this call would have found its own lock above.
+      return;
+    }
+    await unlink(capturePath).catch((error) => {
+      if (errorCode(error) !== "ENOENT") throw error;
+    });
+  } finally {
+    await unlink(ownPinPath).catch((error) => {
       if (errorCode(error) !== "ENOENT") throw error;
     });
   }