fix(runner): make ACPX stale-gate removal atomic and unflake its CI test

Stale-gate recovery proved a gate's holder dead from a pinned copy, then
checked the shared path's identity with one lstat call and removed it with a
separate, later unlink call. A fresh claimant's gate landing in the gap
between those two calls was removed right along with the dead gate, letting
a claim and a teardown run at the same time on one sandbox root.

Recovery now removes the shared gate with a single rename, which atomically
vacates the shared path and captures whatever it names at that instant. Only
after the rename does recovery compare what it caught against the gate it
already proved dead: a match deletes it for good, and a mismatch restores
the caught, still-live gate immediately with link, which fails instead of
overwriting a third claimant that grabbed the path first.

The CI Build check also failed a related test on a fixed timing assumption:
it moved a concurrently kicked-off claim forward with a short, real-time
wait before reading the gate's content. A loaded CI runner can push the
underlying filesystem calls well past that wait, matching a known timing
gap already documented in this driver's host test file. Both content
assertions in this file now poll for the expected state instead, bounded by
a generous named deadline.

A new test exercises the one race recovery's atomic removal cannot close on
its own: a third claimant that grabs the shared path in the instant the
capturing rename vacates it. Recovery must never clobber that claim, and
must never lose the live gate it caught either.

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

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

@@ -38,6 +38,21 @@ function waitForConcurrentClaimHeadStart(): Promise<void> {
   );
 }
 
+// A gate recovery attempt this file races against runs through several real
+// filesystem calls (link, lstat, readFile, rename) before it settles into a
+// stable state. A loaded CI runner can push that well past a short fixed
+// delay, so every assertion in this file that waits for recovery to reach a
+// specific gate state polls for it instead, bounded by this deadline.
+const ACPX_SANDBOX_GATE_STATE_TIMEOUT_MS = 10_000;
+
+async function waitForAcpxSandboxGateState<T>(
+  check: () => T | Promise<T>,
+): Promise<T> {
+  return await vi.waitFor(check, {
+    timeout: ACPX_SANDBOX_GATE_STATE_TIMEOUT_MS,
+  });
+}
+
 const temporaryDirectories: string[] = [];
 
 afterEach(async () => {
@@ -729,10 +744,14 @@ describe("ACPX runtime sandbox", () => {
     // Finding anything other than this exact marker — the file gone, or a
     // fresh gate holding only this process's own acquisition token — proves
     // recovery removed the live replacement it found in place of the dead
-    // gate it inspected.
-    await expect(readFile(gatePath, "utf8")).resolves.toBe(
-      replacementGateContents,
-    );
+    // gate it inspected. Recovery reaches this state through several real
+    // filesystem calls after the head start above, so this polls for it
+    // rather than assuming the head start alone was enough time.
+    await waitForAcpxSandboxGateState(async () => {
+      await expect(readFile(gatePath, "utf8")).resolves.toBe(
+        replacementGateContents,
+      );
+    });
 
     // Free the replacement gate, as its own holder would on completing its
     // claim, so the still-retrying recovery call above can proceed.
@@ -803,10 +822,15 @@ describe("ACPX runtime sandbox", () => {
     expect(concurrentClaimSettled).toBe(false);
     // Finding anything other than this exact marker proves recovery — or
     // the concurrent claim's own recovery attempt — removed the live
-    // replacement instead of continuing to retry around it.
-    await expect(readFile(gatePath, "utf8")).resolves.toBe(
-      replacementGateContents,
-    );
+    // replacement instead of continuing to retry around it. Both retry
+    // attempts reach this state through several real filesystem calls after
+    // the head start above, so this polls for it rather than assuming the
+    // head start alone was enough time.
+    await waitForAcpxSandboxGateState(async () => {
+      await expect(readFile(gatePath, "utf8")).resolves.toBe(
+        replacementGateContents,
+      );
+    });
 
     // Free the replacement gate, as its own holder would on completing its
     // claim, so both still-retrying calls above can proceed.
@@ -823,6 +847,81 @@ describe("ACPX runtime sandbox", () => {
     );
     await recoveringProcess.removeOwnedAcpxRuntimeSandboxRoot(sandbox);
   });
+
+  it("never clobbers a third claimant's gate when recovery cannot restore a captured live replacement", 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`);
+
+    // 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"]);
+    await writeFile(gatePath, String(deadHolder.pid), { flag: "wx" });
+
+    vi.resetModules();
+    const recoveringProcess: typeof import("./runtime-sandbox.js") =
+      await import("./runtime-sandbox.js");
+
+    // Recovery proves the dead gate stale, then atomically captures
+    // whatever now sits at the shared path with a single `rename`, which
+    // briefly vacates that path. This test places a live replacement there
+    // first, so the capture catches that replacement instead of the dead
+    // gate, then, in the instant the rename leaves the path empty, places a
+    // third claimant's own gate there before recovery's restore can land —
+    // the one case recovery cannot make race-free, because a genuine new
+    // claim through the shared path's own exclusivity primitive is
+    // indistinguishable from any other. Recovery must still never clobber
+    // that third claimant's gate, and must never lose the replacement it
+    // caught either.
+    const replacementGateContents = `${process.pid}:live-replacement`;
+    const thirdClaimantGateContents = `${process.pid}:third-claimant`;
+    const recovered = recoveringProcess.prepareAcpxRuntimeSandbox(
+      { binding: fixture.binding, agent: "codex" },
+      {
+        beforeStaleGateRemoval: async () => {
+          await unlink(gatePath);
+          await writeFile(gatePath, replacementGateContents, { flag: "wx" });
+        },
+        afterStaleGateCapture: async () => {
+          await writeFile(gatePath, thirdClaimantGateContents, {
+            flag: "wx",
+          });
+        },
+      },
+    );
+
+    // Recovery's restore attempt reaches this state through several real
+    // filesystem calls, so this polls for it rather than assuming a fixed
+    // delay was enough time.
+    await waitForAcpxSandboxGateState(async () => {
+      await expect(readFile(gatePath, "utf8")).resolves.toBe(
+        thirdClaimantGateContents,
+      );
+    });
+
+    // The captured live replacement was not silently discarded: it is left
+    // behind under its own private name, off the shared path, exactly
+    // where the capturing rename first put it.
+    const strandedCaptures = (await readdir(dirname(gatePath))).filter(
+      (entry) => entry.includes(".reap-"),
+    );
+    expect(strandedCaptures).toHaveLength(1);
+    await expect(
+      readFile(join(dirname(gatePath), strandedCaptures[0]), "utf8"),
+    ).resolves.toBe(replacementGateContents);
+
+    // Free the third claimant's gate, as its own holder would on completing
+    // its claim, so the still-retrying recovery call above can proceed.
+    await unlink(gatePath);
+
+    const sandbox = await recovered;
+    expect(sandbox.root).toBe(probe.root);
+    await recoveringProcess.removeOwnedAcpxRuntimeSandboxRoot(sandbox);
+  });
 });
 
 async function sandboxFixture(agent: "pi" | "claude" | "codex") {

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

@@ -418,6 +418,14 @@ export interface AcpxRuntimeSandboxPrepareDependencies {
    * never by disturbing the shared path first.
    */
   beforeStaleGateRemoval?: () => Promise<void>;
+  /**
+   * Internal test seam. Runs once stale-gate recovery has atomically
+   * captured a live replacement gate in place of the dead one it proved
+   * stale, immediately before recovery restores that capture to the shared
+   * path. Lets a test claim the now-momentarily-empty shared path first, to
+   * prove recovery's restore never clobbers that claim.
+   */
+  afterStaleGateCapture?: () => Promise<void>;
 }
 
 /** Prepare the private filesystem and environment visible to an ACPX agent. */
@@ -740,6 +748,10 @@ function acpxRuntimeSandboxRootGatePath(root: string): string {
 
 interface AcpxRuntimeSandboxRootGateDependencies {
   beforeStaleGateRemoval?: () => Promise<void>;
+  /** Internal seam for racing a third claimant into the moment between the
+   * atomic capture and the restore in `removeStaleAcpxRuntimeSandboxRootGate`,
+   * exercised only in tests. */
+  afterStaleGateCapture?: () => Promise<void>;
 }
 
 /**
@@ -849,18 +861,33 @@ async function acquireAcpxRuntimeSandboxRootGate(
 // This function instead pins whatever currently occupies the shared path
 // with an extra `link` first. Unlike `rename`, `link` adds a second name for
 // the same inode without ever removing the first, so the shared path is
-// never vacated: a genuinely live holder's gate stays exclusive there for
-// the holder's entire critical section, no matter how this call's inspection
-// of its own pinned copy turns out. The pin also keeps that exact inode
-// alive for as long as this call holds it, so a later identity check against
-// it can never be confused by a filesystem reusing a freed inode number for
-// an unrelated new file at the same path — the well-known hazard a bare
-// device-and-inode check runs into (observed on ext4). Only once this call
-// has proved, from its own pinned copy, that the holder is dead does it
-// remove the shared name — and only if the shared path still names that
-// exact pinned inode at that moment. A live holder that claims the path
-// meanwhile always creates a fresh file, so it can never land on this call's
-// already-pinned identity, and this call never removes it.
+// never vacated while this call decides whether the holder is dead: a
+// genuinely live holder's gate stays exclusive there for the holder's entire
+// critical section, no matter how this call's inspection of its own pinned
+// copy turns out. The pin also keeps that exact inode alive for as long as
+// this call holds it, so a later identity check against it can never be
+// confused by a filesystem reusing a freed inode number for an unrelated new
+// file at the same path — the well-known hazard a bare device-and-inode
+// check runs into (observed on ext4).
+//
+// Once this call has proved, from its own pinned copy, that the holder is
+// dead, it still must remove only that exact dead gate, never a fresh one
+// that has since replaced it. A separate identity check followed by a
+// separate pathname `unlink` cannot prove that: whatever the check reads,
+// the `unlink` call still removes whatever the path names at its own later
+// moment, not what the check read, so a fresh claimant's gate landing in
+// that gap is removed right along with the dead one. This call closes that
+// gap by combining the check and the removal into one `rename`, which
+// atomically vacates the shared path and pulls in whatever it named at that
+// exact instant. Only after the rename does this call compare what it
+// caught against the pinned identity: a match proves it caught the same
+// dead gate it already proved stale, safe to delete for good. A mismatch
+// means a fresh claimant's gate was caught instead, so this call restores it
+// immediately with `link`, not `rename` — `link` fails instead of silently
+// overwriting a third claimant that grabbed the momentarily empty path
+// first, so that third claimant's own gate is never clobbered by the
+// restore, and the caught gate is kept under its own private name rather
+// than lost.
 async function breakStaleAcpxRuntimeSandboxRootGate(
   gatePath: string,
   dependencies: AcpxRuntimeSandboxRootGateDependencies = {},
@@ -898,28 +925,11 @@ async function breakStaleAcpxRuntimeSandboxRootGate(
       }
     }
     await dependencies.beforeStaleGateRemoval?.();
-
-    const sharedEntry = await lstat(gatePath, { bigint: true }).catch(
-      (error) => {
-        if (errorCode(error) === "ENOENT") return null;
-        throw error;
-      },
+    await removeStaleAcpxRuntimeSandboxRootGate(
+      gatePath,
+      pinnedEntry,
+      dependencies,
     );
-    if (
-      sharedEntry !== null &&
-      sharedEntry.dev === pinnedEntry.dev &&
-      sharedEntry.ino === pinnedEntry.ino
-    ) {
-      // The shared path still names the exact dead gate this call pinned
-      // and proved stale: safe to remove.
-      await unlink(gatePath).catch((error) => {
-        if (errorCode(error) !== "ENOENT") throw error;
-      });
-    }
-    // A mismatch means a live holder already claimed the shared path with a
-    // fresh gate before this call reached its identity check. This call
-    // never touched the shared path, so that live gate was never disturbed
-    // and there is nothing to restore.
   } finally {
     await unlink(capturePath).catch((error) => {
       if (errorCode(error) !== "ENOENT") throw error;
@@ -927,6 +937,56 @@ async function breakStaleAcpxRuntimeSandboxRootGate(
   }
 }
 
+// Removes the shared gate at `gatePath`, but only the exact dead gate
+// `pinnedEntry` already proved stale, never a fresh gate that has since
+// replaced it. `rename` atomically vacates the shared path and captures
+// whatever it named at that instant, in one call, so there is no separate
+// moment between an identity check and a removal for a fresh claimant's
+// gate to land in.
+async function removeStaleAcpxRuntimeSandboxRootGate(
+  gatePath: string,
+  pinnedEntry: BigIntStats,
+  dependencies: AcpxRuntimeSandboxRootGateDependencies,
+): Promise<void> {
+  const removalPath = `${gatePath}${ACPX_SANDBOX_ROOT_GATE_REAP_INFIX}${process.pid}-${randomBytes(8).toString("hex")}`;
+  try {
+    await rename(gatePath, removalPath);
+  } catch (error) {
+    // Already gone: nothing to remove.
+    if (errorCode(error) === "ENOENT") return;
+    throw error;
+  }
+  const capturedEntry = await lstat(removalPath, { bigint: true });
+  if (
+    capturedEntry.dev === pinnedEntry.dev &&
+    capturedEntry.ino === pinnedEntry.ino
+  ) {
+    // The rename caught the exact dead gate this call already proved
+    // stale: safe to delete for good.
+    await unlink(removalPath).catch((error) => {
+      if (errorCode(error) !== "ENOENT") throw error;
+    });
+    return;
+  }
+  // The rename caught a live replacement a fresh claimant put at the shared
+  // path after this call proved the original dead: put it straight back.
+  await dependencies.afterStaleGateCapture?.();
+  try {
+    await link(removalPath, gatePath);
+  } catch (error) {
+    if (errorCode(error) !== "EEXIST") throw error;
+    // A third claimant grabbed the shared path in the brief moment this
+    // call's rename vacated it, before the restore could land. That
+    // claimant's gate must not be clobbered, so the caught gate is left
+    // behind under its own private name instead of being forced back or
+    // deleted.
+    return;
+  }
+  await unlink(removalPath).catch((error) => {
+    if (errorCode(error) !== "ENOENT") throw error;
+  });
+}
+
 function isAcpxSandboxRootGateHolderAlive(pid: number): boolean {
   try {
     process.kill(pid, 0);