fix(runner): close the ACPX sandbox ownership check race

A claim (marker write plus lease registration) and a teardown (marker
read, lease count, then root delete or marker release) each read state
and then act on it, as two separate filesystem calls. A concurrent
claim could land in the gap between a teardown's read and its act: the
teardown read zero other leases, a new admission then registered a
lease and started using the root, and the teardown deleted or released
that root anyway.

Add a per-root gate file, opened with O_CREAT|O_EXCL like the marker
claim, so a claim and a teardown for the same root run one at a time
across every Runner process. A stale gate left by a crashed process is
broken only when its holder process is provably gone.

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

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

@@ -4,6 +4,7 @@ import {
   mkdir,
   mkdtemp,
   open,
+  readdir,
   readFile,
   rename,
   rm,
@@ -12,7 +13,7 @@ import {
   writeFile,
 } from "node:fs/promises";
 import { tmpdir } from "node:os";
-import { dirname, join } from "node:path";
+import { basename, dirname, join } from "node:path";
 
 import { afterEach, describe, expect, it, vi } from "vitest";
 
@@ -25,6 +26,16 @@ import {
   removeOwnedAcpxRuntimeSandboxRoot,
 } from "./runtime-sandbox.js";
 
+const CONCURRENT_CLAIM_HEAD_START_MS = 25;
+
+/** Gives a concurrently kicked-off claim a real chance to run ahead, so a
+ * later "did not settle yet" check is meaningful rather than incidental. */
+function waitForConcurrentClaimHeadStart(): Promise<void> {
+  return new Promise((resolve) =>
+    setTimeout(resolve, CONCURRENT_CLAIM_HEAD_START_MS),
+  );
+}
+
 const temporaryDirectories: string[] = [];
 
 afterEach(async () => {
@@ -400,6 +411,118 @@ describe("ACPX runtime sandbox", () => {
     );
     await expect(stat(owner.root)).resolves.toBeDefined();
   });
+
+  it("never frees a marker for a concurrent claim to slip past a still-deciding release", async () => {
+    const fixture = await sandboxFixture("codex");
+    const first = await prepareAcpxRuntimeSandbox({
+      binding: fixture.binding,
+      agent: "codex",
+    });
+    const credentialPath = join(first.agentHomeDirectory, "auth.json");
+    await writeFile(credentialPath, '{"owner":"first"}\n');
+
+    let concurrentProcess: typeof import("./runtime-sandbox.js") | null =
+      null;
+    let concurrentClaim: ReturnType<typeof prepareAcpxRuntimeSandbox> | null =
+      null;
+    let concurrentClaimSettled = false;
+
+    // The owner admission closes normally. Once it has decided the marker
+    // is free to release, race a second, concurrent admission's claim for
+    // the same deterministic root against that decision, simulated as a
+    // second Runner process.
+    await releaseAcpxRuntimeSandboxRootClaim(first, {
+      afterLeaseCountDecision: async () => {
+        vi.resetModules();
+        concurrentProcess = await import("./runtime-sandbox.js");
+        concurrentClaim = concurrentProcess
+          .prepareAcpxRuntimeSandbox({
+            binding: fixture.binding,
+            agent: "codex",
+          })
+          .finally(() => {
+            concurrentClaimSettled = true;
+          });
+        await waitForConcurrentClaimHeadStart();
+        // The concurrent claim's own marker write needs this admission's
+        // ownership gate, which this admission still holds here.
+        expect(concurrentClaimSettled).toBe(false);
+      },
+    });
+
+    const second = await concurrentClaim!;
+    expect(second.root).toBe(first.root);
+    // The concurrent claim only proceeded once the release had fully
+    // finished, so it became this root's sole new owner with a fresh
+    // ownership marker, rather than sharing a marker this admission was
+    // about to free regardless of who else was using the root.
+    await concurrentProcess!.removeOwnedAcpxRuntimeSandboxRoot(second);
+    await expect(stat(first.root)).rejects.toMatchObject({ code: "ENOENT" });
+
+    const siblingEntries = await readdir(dirname(first.root));
+    expect(siblingEntries).not.toContain(`${basename(first.root)}.gate`);
+  });
+
+  it("keeps a concurrent claim's own outcome well-defined when it races an in-flight abort's delete decision", async () => {
+    const fixture = await sandboxFixture("codex");
+    const first = await prepareAcpxRuntimeSandbox({
+      binding: fixture.binding,
+      agent: "codex",
+    });
+
+    let concurrentProcess: typeof import("./runtime-sandbox.js") | null =
+      null;
+    let concurrentClaim: ReturnType<typeof prepareAcpxRuntimeSandbox> | null =
+      null;
+    let concurrentClaimSettled = false;
+
+    // Nobody else is using the root, so this abort will delete it. Once it
+    // has made that decision, race a second, concurrent admission's claim
+    // for the same deterministic root against the delete itself, simulated
+    // as a second Runner process.
+    await removeOwnedAcpxRuntimeSandboxRoot(first, {
+      afterLeaseCountDecision: async () => {
+        vi.resetModules();
+        concurrentProcess = await import("./runtime-sandbox.js");
+        concurrentClaim = concurrentProcess
+          .prepareAcpxRuntimeSandbox({
+            binding: fixture.binding,
+            agent: "codex",
+          })
+          .finally(() => {
+            concurrentClaimSettled = true;
+          });
+        await waitForConcurrentClaimHeadStart();
+        // The concurrent claim's own marker write needs this admission's
+        // ownership gate, which this admission still holds here.
+        expect(concurrentClaimSettled).toBe(false);
+      },
+    });
+
+    // The concurrent claim was deferred until after this admission's delete
+    // fully finished, so it never raced the delete itself; the deleted root
+    // was never live for it to lose. It either lost the root outright and
+    // failed cleanly, or it rebuilt a complete fresh one and succeeded
+    // cleanly — never a half-built directory silently reported as ready.
+    const outcome = await concurrentClaim!.then(
+      (sandbox) => ({ ok: true as const, sandbox }),
+      (error: unknown) => ({ ok: false as const, error }),
+    );
+    if (outcome.ok) {
+      expect(outcome.sandbox.root).toBe(first.root);
+      await expect(
+        stat(outcome.sandbox.stateDirectory),
+      ).resolves.toBeDefined();
+      await concurrentProcess!.removeOwnedAcpxRuntimeSandboxRoot(
+        outcome.sandbox,
+      );
+    } else {
+      expect(outcome.error).toBeInstanceOf(Error);
+    }
+
+    const siblingEntries = await readdir(dirname(first.root));
+    expect(siblingEntries).not.toContain(`${basename(first.root)}.gate`);
+  });
 });
 
 async function sandboxFixture(agent: "pi" | "claude" | "codex") {

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

@@ -80,7 +80,11 @@ const acpxRuntimeSandboxRootOwners = new Map<
 // root or releases the marker, so a still-live admission in another process
 // is never deleted out from under, and the marker is never freed for reclaim
 // while that admission still holds a lease. Each admission removes only its
-// own lease file, when its own use of the root ends.
+// own lease file, when its own use of the root ends. Reading the lease files
+// and acting on what they show are two separate filesystem calls; a
+// same-root gate (defined further down, beside the lease helpers) makes a
+// claim's own lease registration and a teardown's read-then-act run one at a
+// time, so a new lease can never land in the gap between them.
 
 // Associates each returned sandbox object with the exact owner it was built
 // under, without exposing the ownership identifier on the public sandbox
@@ -95,6 +99,21 @@ const ACPX_SANDBOX_ROOT_MARKER_MAX_BYTES = 128;
 const ACPX_SANDBOX_ROOT_LEASE_INFIX = ".lease-";
 const ACPX_SANDBOX_ROOT_LEASE_IDENTIFIER_PATTERN = /^[0-9a-f]{32}$/;
 
+// A claim (marker write plus lease registration) and a teardown (marker
+// read, lease count, then root delete or marker release) each read state and
+// then act on it. Reading and acting are two separate filesystem calls, so
+// without more, a claim can land in the gap between a teardown's read and
+// its act: the teardown reads zero other leases, a new admission then
+// registers a lease and starts using the root, and the teardown deletes that
+// root anyway. This gate makes every claim and every teardown, for the same
+// root, run one at a time, so a teardown's read and act always observe the
+// same lease set and no claim can land inside that gap. It guards only this
+// short read-then-act critical section, never the slower directory build
+// that follows a claim.
+const ACPX_SANDBOX_ROOT_GATE_SUFFIX = ".gate";
+const ACPX_SANDBOX_ROOT_GATE_ACQUIRE_TIMEOUT_MS = 20_000;
+const ACPX_SANDBOX_ROOT_GATE_RETRY_DELAY_MS = 15;
+
 export interface AcpxRuntimeSandbox {
   root: string;
   stateDirectory: string;
@@ -690,6 +709,124 @@ async function countOtherAcpxRuntimeSandboxRootLeases(
   }).length;
 }
 
+function acpxRuntimeSandboxRootGatePath(root: string): string {
+  return join(
+    dirname(root),
+    `${basename(root)}${ACPX_SANDBOX_ROOT_GATE_SUFFIX}`,
+  );
+}
+
+/**
+ * Run `criticalSection` as the sole holder of the gate for `root`, across
+ * every Runner process. The gate is a file created with O_CREAT|O_EXCL, the
+ * same cross-process exclusivity primitive the marker claim uses. Call this
+ * around a claim's marker write plus lease registration, and around a
+ * teardown's lease read plus its resulting delete or marker release, so a
+ * claim and a teardown for the same root can never interleave.
+ */
+async function withAcpxRuntimeSandboxRootGate<T>(
+  root: string,
+  criticalSection: () => Promise<T>,
+): Promise<T> {
+  const gatePath = acpxRuntimeSandboxRootGatePath(root);
+  const deadline = Date.now() + ACPX_SANDBOX_ROOT_GATE_ACQUIRE_TIMEOUT_MS;
+  for (;;) {
+    if (await acquireAcpxRuntimeSandboxRootGate(gatePath)) {
+      try {
+        return await criticalSection();
+      } finally {
+        await unlink(gatePath).catch((error) => {
+          if (errorCode(error) !== "ENOENT") throw error;
+        });
+      }
+    }
+    // The gate is held by another admission. Break it only when that holder
+    // is provably gone (a crashed process), never on a mere timeout, so a
+    // slow-but-live holder is never pre-empted.
+    await breakStaleAcpxRuntimeSandboxRootGate(gatePath);
+    if (Date.now() >= deadline) {
+      throw new Error(
+        "ACPX sandbox root ownership gate could not be acquired",
+      );
+    }
+    await delay(ACPX_SANDBOX_ROOT_GATE_RETRY_DELAY_MS);
+  }
+}
+
+async function acquireAcpxRuntimeSandboxRootGate(
+  gatePath: string,
+): Promise<boolean> {
+  let handle: FileHandle;
+  try {
+    handle = await open(
+      gatePath,
+      constants.O_WRONLY |
+        constants.O_CREAT |
+        constants.O_EXCL |
+        (constants.O_NOFOLLOW ?? 0),
+      PRIVATE_FILE_MODE,
+    );
+  } catch (error) {
+    if (errorCode(error) === "EEXIST") return false;
+    throw error;
+  }
+  try {
+    await handle.chmod(PRIVATE_FILE_MODE);
+    await handle.writeFile(String(process.pid), "utf8");
+    await handle.sync();
+  } finally {
+    await handle.close();
+  }
+  return true;
+}
+
+// Removes a gate file left behind by a holder process that no longer exists,
+// so a hard process crash inside the critical section cannot block every
+// later admission for this root forever. Never removes a gate whose holder
+// is still alive, or one this process cannot prove is dead.
+async function breakStaleAcpxRuntimeSandboxRootGate(
+  gatePath: string,
+): Promise<void> {
+  let holderPid: number | null = null;
+  try {
+    const handle = await open(
+      gatePath,
+      constants.O_RDONLY | (constants.O_NOFOLLOW ?? 0),
+    );
+    try {
+      const bytes = await readFile(handle);
+      const parsed = Number.parseInt(bytes.toString("utf8").trim(), 10);
+      holderPid = Number.isInteger(parsed) && parsed > 0 ? parsed : null;
+    } finally {
+      await handle.close();
+    }
+  } catch {
+    // Missing, unreadable, or malformed: nothing provable to break here.
+    return;
+  }
+  if (holderPid !== null && isAcpxSandboxRootGateHolderAlive(holderPid)) {
+    return;
+  }
+  await unlink(gatePath).catch((error) => {
+    if (errorCode(error) !== "ENOENT") throw error;
+  });
+}
+
+function isAcpxSandboxRootGateHolderAlive(pid: number): boolean {
+  try {
+    process.kill(pid, 0);
+    return true;
+  } catch (error) {
+    // ESRCH proves the process is gone. Any other outcome (for example
+    // EPERM) cannot prove that, so treat the holder as still alive.
+    return errorCode(error) !== "ESRCH";
+  }
+}
+
+function delay(timeoutMs: number): Promise<void> {
+  return new Promise((resolve) => setTimeout(resolve, timeoutMs));
+}
+
 function reportDeclinedAcpxSandboxRootClaim(root: string): void {
   process.emitWarning(
     JSON.stringify({
@@ -719,14 +856,20 @@ async function claimAcpxRuntimeSandboxRoot(
     dev: entry.dev,
     ino: entry.ino,
   };
-  const claimed = await writeAcpxSandboxRootMarkerExclusive(
-    markerPath,
-    identifier,
-  );
-  // Register this admission's own durable lease before returning, whether
-  // or not it won the marker, so its occupancy is visible to any process
-  // that later reads the root's leases — including this one.
-  await registerAcpxRuntimeSandboxRootLease(root, identifier);
+  // The marker write and the lease registration run as one gated step, so a
+  // concurrent teardown's lease count can never land between them and miss
+  // this admission's occupancy.
+  const claimed = await withAcpxRuntimeSandboxRootGate(root, async () => {
+    const won = await writeAcpxSandboxRootMarkerExclusive(
+      markerPath,
+      identifier,
+    );
+    // Register this admission's own durable lease before returning, whether
+    // or not it won the marker, so its occupancy is visible to any process
+    // that later reads the root's leases — including this one.
+    await registerAcpxRuntimeSandboxRootLease(root, identifier);
+    return won;
+  });
   if (!claimed) {
     // Another admission already owns this deterministic root. Continue
     // without delete authority: never overwrite its marker, never touch the
@@ -747,61 +890,79 @@ async function claimAcpxRuntimeSandboxRoot(
  * identifier, a changed identity, or another admission still sharing the
  * root — proved by a durable lease file, so a sharing admission in a
  * different process is seen too — means: delete nothing.
+ *
+ * The lease read, the identity revalidation, and the resulting delete or
+ * retain all run as one gated step, so no claim can register a new lease in
+ * the gap between this admission counting leases and acting on that count.
  */
 async function revalidateAndRemoveOwnedAcpxRuntimeSandboxRoot(
   root: string,
   owner: AcpxRuntimeSandboxRootOwner,
+  dependencies: AcpxRuntimeSandboxRootTeardownDependencies = {},
 ): Promise<void> {
-  // This admission's own use of the root ends here, whether or not it goes
-  // on to hold delete authority.
-  await releaseAcpxRuntimeSandboxRootLease(root, owner.identifier);
+  await withAcpxRuntimeSandboxRootGate(root, async () => {
+    // This admission's own use of the root ends here, whether or not it goes
+    // on to hold delete authority.
+    await releaseAcpxRuntimeSandboxRootLease(root, owner.identifier);
 
-  const storedIdentifier = await readAcpxSandboxRootMarkerIdentifier(
-    owner.markerPath,
-  );
-  if (storedIdentifier !== owner.identifier) {
-    // A missing marker or a different identifier means another admission
-    // owns (or once owned) this root, or this admission's own claim was
-    // declined. Either way, delete nothing.
+    const storedIdentifier = await readAcpxSandboxRootMarkerIdentifier(
+      owner.markerPath,
+    );
+    if (storedIdentifier !== owner.identifier) {
+      // A missing marker or a different identifier means another admission
+      // owns (or once owned) this root, or this admission's own claim was
+      // declined. Either way, delete nothing.
+      if (acpxRuntimeSandboxRootOwners.get(root) === owner) {
+        acpxRuntimeSandboxRootOwners.delete(root);
+      }
+      return;
+    }
+    let entry: BigIntStats;
+    try {
+      entry = await lstat(root, { bigint: true });
+    } catch (error) {
+      if (errorCode(error) === "ENOENT") {
+        acpxRuntimeSandboxRootOwners.delete(root);
+        return;
+      }
+      throw error;
+    }
+    if (
+      entry.isSymbolicLink() ||
+      !entry.isDirectory() ||
+      entry.dev !== owner.dev ||
+      entry.ino !== owner.ino
+    ) {
+      throw new Error("ACPX sandbox root identity changed before cleanup");
+    }
+    if (
+      (await countOtherAcpxRuntimeSandboxRootLeases(root, owner.identifier)) >
+      0
+    ) {
+      // Another admission — possibly in a different Runner process — built a
+      // live sandbox on this same root and has not yet released its lease.
+      // Leave the root, the marker, and the registry entry alone: the
+      // failure mode is a retained directory, never a deleted live one.
+      return;
+    }
+    await dependencies.afterLeaseCountDecision?.();
+    await rm(root, { recursive: true });
+    await unlink(owner.markerPath).catch((error) => {
+      if (errorCode(error) !== "ENOENT") throw error;
+    });
     if (acpxRuntimeSandboxRootOwners.get(root) === owner) {
       acpxRuntimeSandboxRootOwners.delete(root);
     }
-    return;
-  }
-  let entry: BigIntStats;
-  try {
-    entry = await lstat(root, { bigint: true });
-  } catch (error) {
-    if (errorCode(error) === "ENOENT") {
-      acpxRuntimeSandboxRootOwners.delete(root);
-      return;
-    }
-    throw error;
-  }
-  if (
-    entry.isSymbolicLink() ||
-    !entry.isDirectory() ||
-    entry.dev !== owner.dev ||
-    entry.ino !== owner.ino
-  ) {
-    throw new Error("ACPX sandbox root identity changed before cleanup");
-  }
-  if (
-    (await countOtherAcpxRuntimeSandboxRootLeases(root, owner.identifier)) > 0
-  ) {
-    // Another admission — possibly in a different Runner process — built a
-    // live sandbox on this same root and has not yet released its lease.
-    // Leave the root, the marker, and the registry entry alone: the failure
-    // mode is a retained directory, never a deleted live one.
-    return;
-  }
-  await rm(root, { recursive: true });
-  await unlink(owner.markerPath).catch((error) => {
-    if (errorCode(error) !== "ENOENT") throw error;
   });
-  if (acpxRuntimeSandboxRootOwners.get(root) === owner) {
-    acpxRuntimeSandboxRootOwners.delete(root);
-  }
+}
+
+export interface AcpxRuntimeSandboxRootTeardownDependencies {
+  /**
+   * Internal test seam. Runs inside the root's ownership gate, once this
+   * admission has decided to act (delete the root, or free the marker) and
+   * before it does so.
+   */
+  afterLeaseCountDecision?: () => Promise<void>;
 }
 
 /**
@@ -812,12 +973,17 @@ async function revalidateAndRemoveOwnedAcpxRuntimeSandboxRoot(
  */
 export async function removeOwnedAcpxRuntimeSandboxRoot(
   sandbox: AcpxRuntimeSandbox,
+  dependencies: AcpxRuntimeSandboxRootTeardownDependencies = {},
 ): Promise<void> {
   const owner = acpxRuntimeSandboxRootOwnerByResult.get(sandbox);
   if (!owner) {
     throw new Error("ACPX sandbox root ownership capability is unavailable");
   }
-  await revalidateAndRemoveOwnedAcpxRuntimeSandboxRoot(owner.root, owner);
+  await revalidateAndRemoveOwnedAcpxRuntimeSandboxRoot(
+    owner.root,
+    owner,
+    dependencies,
+  );
 }
 
 /**
@@ -838,34 +1004,42 @@ export async function removeOwnedAcpxRuntimeSandboxRoot(
  * to admissions that share it; the marker then remains stale until an
  * existing recovery process reclaims it, the same accepted failure mode as
  * a marker left by a hard process crash.
+ *
+ * The lease read, the lease count, and the resulting marker release all run
+ * as one gated step, so no claim can register a new lease in the gap
+ * between this admission counting leases and freeing the marker.
  */
 export async function releaseAcpxRuntimeSandboxRootClaim(
   sandbox: AcpxRuntimeSandbox,
+  dependencies: AcpxRuntimeSandboxRootTeardownDependencies = {},
 ): Promise<void> {
   const owner = acpxRuntimeSandboxRootOwnerByResult.get(sandbox);
   if (!owner) {
     throw new Error("ACPX sandbox root ownership capability is unavailable");
   }
-  // This admission's own use of the root ends here.
-  await releaseAcpxRuntimeSandboxRootLease(owner.root, owner.identifier);
+  await withAcpxRuntimeSandboxRootGate(owner.root, async () => {
+    // This admission's own use of the root ends here.
+    await releaseAcpxRuntimeSandboxRootLease(owner.root, owner.identifier);
 
-  const storedIdentifier = await readAcpxSandboxRootMarkerIdentifier(
-    owner.markerPath,
-  );
-  if (
-    storedIdentifier === owner.identifier &&
-    (await countOtherAcpxRuntimeSandboxRootLeases(
-      owner.root,
-      owner.identifier,
-    )) === 0
-  ) {
-    await unlink(owner.markerPath).catch((error) => {
-      if (errorCode(error) !== "ENOENT") throw error;
-    });
-  }
-  if (acpxRuntimeSandboxRootOwners.get(owner.root) === owner) {
-    acpxRuntimeSandboxRootOwners.delete(owner.root);
-  }
+    const storedIdentifier = await readAcpxSandboxRootMarkerIdentifier(
+      owner.markerPath,
+    );
+    const freeable =
+      storedIdentifier === owner.identifier &&
+      (await countOtherAcpxRuntimeSandboxRootLeases(
+        owner.root,
+        owner.identifier,
+      )) === 0;
+    if (freeable) {
+      await dependencies.afterLeaseCountDecision?.();
+      await unlink(owner.markerPath).catch((error) => {
+        if (errorCode(error) !== "ENOENT") throw error;
+      });
+    }
+    if (acpxRuntimeSandboxRootOwners.get(owner.root) === owner) {
+      acpxRuntimeSandboxRootOwners.delete(owner.root);
+    }
+  });
 }
 
 async function ensurePrivateDirectory(