diff --git a/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.test.ts b/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.test.ts index 32ba6a6bdec..51b56fd4eb8 100644 --- a/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.test.ts +++ b/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.test.ts @@ -1,3 +1,4 @@ +import { spawnSync } from "node:child_process"; import { chmod, lstat, @@ -10,6 +11,7 @@ import { rm, stat, symlink, + unlink, writeFile, } from "node:fs/promises"; import { tmpdir } from "node:os"; @@ -523,6 +525,85 @@ describe("ACPX runtime sandbox", () => { const siblingEntries = await readdir(dirname(first.root)); expect(siblingEntries).not.toContain(`${basename(first.root)}.gate`); }); + + it("recovers a gate left behind by a holder process that no longer exists", 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" }); + + const recovered = await prepareAcpxRuntimeSandbox({ + binding: fixture.binding, + agent: "codex", + }); + expect(recovered.root).toBe(probe.root); + await expect(stat(gatePath)).rejects.toMatchObject({ code: "ENOENT" }); + await removeOwnedAcpxRuntimeSandboxRoot(recovered); + }); + + it("never removes a live gate that replaced the stale gate recovery inspected", 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"); + + // Once recovery has read the dead gate and proved its holder is gone, + // but before it removes the gate by pathname, replace the file at that + // same path with a fresh, live gate — simulating a second admission + // that broke in on the same dead gate first and is now using it. Its + // content names this test process, the same real process the + // recovering call also runs in, plus a marker suffix production code + // never writes itself: a plain pid alone cannot tell this exact + // replacement apart from a later gate the recovering call might create + // for its own use, since both would then name the same pid. + const replacementGateContents = `${process.pid}:live-replacement`; + const recovered = recoveringProcess.prepareAcpxRuntimeSandbox( + { binding: fixture.binding, agent: "codex" }, + { + beforeStaleGateRemoval: async () => { + await unlink(gatePath); + await writeFile(gatePath, replacementGateContents, { flag: "wx" }); + }, + }, + ); + + await waitForConcurrentClaimHeadStart(); + // 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, + ); + + // Free the replacement 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") { diff --git a/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.ts b/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.ts index c3fd0184b11..6553b4d839e 100644 --- a/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.ts +++ b/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.ts @@ -396,6 +396,14 @@ export interface AcpxRuntimeSandboxPrepareDependencies { * before the rest of the sandbox is built. */ afterRootOwned?: () => Promise; + /** + * Internal test seam. Runs after stale-gate recovery has read a gate file + * and proved its holder is dead, immediately before recovery re-checks + * the gate's identity and removes it. Lets a test replace the gate at + * this same path in that gap, to prove the removal that follows targets + * only the exact gate instance recovery inspected. + */ + beforeStaleGateRemoval?: () => Promise; } /** Prepare the private filesystem and environment visible to an ACPX agent. */ @@ -431,7 +439,7 @@ export async function prepareAcpxRuntimeSandbox( // this admission continues without delete authority, and its own cleanup // later finds the marker does not carry its identifier and leaves the // live root alone. - const owner = await claimAcpxRuntimeSandboxRoot(root); + const owner = await claimAcpxRuntimeSandboxRoot(root, dependencies); try { await dependencies.afterRootOwned?.(); const stateDirectory = await ensurePrivateDirectory( @@ -716,6 +724,10 @@ function acpxRuntimeSandboxRootGatePath(root: string): string { ); } +interface AcpxRuntimeSandboxRootGateDependencies { + beforeStaleGateRemoval?: () => Promise; +} + /** * 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 @@ -727,6 +739,7 @@ function acpxRuntimeSandboxRootGatePath(root: string): string { async function withAcpxRuntimeSandboxRootGate( root: string, criticalSection: () => Promise, + dependencies: AcpxRuntimeSandboxRootGateDependencies = {}, ): Promise { const gatePath = acpxRuntimeSandboxRootGatePath(root); const deadline = Date.now() + ACPX_SANDBOX_ROOT_GATE_ACQUIRE_TIMEOUT_MS; @@ -743,7 +756,7 @@ async function withAcpxRuntimeSandboxRootGate( // 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); + await breakStaleAcpxRuntimeSandboxRootGate(gatePath, dependencies); if (Date.now() >= deadline) { throw new Error( "ACPX sandbox root ownership gate could not be acquired", @@ -772,7 +785,17 @@ async function acquireAcpxRuntimeSandboxRootGate( } try { await handle.chmod(PRIVATE_FILE_MODE); - await handle.writeFile(String(process.pid), "utf8"); + // The holder pid, plus a random per-acquisition token so this exact + // gate instance can be told apart from a same-pid replacement created + // at the same path later. A device-and-inode check cannot do this: a + // delete immediately followed by a create at the same path can reuse + // the just-freed inode number on some filesystems (observed on ext4), + // so a replacement gate can carry the very same identity the deleted + // one had. + await handle.writeFile( + `${process.pid}:${randomBytes(16).toString("hex")}`, + "utf8", + ); await handle.sync(); } finally { await handle.close(); @@ -784,10 +807,26 @@ async function acquireAcpxRuntimeSandboxRootGate( // 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. +// +// Another admission can replace the gate at this same path — release the +// stale one and create a fresh, live one — in the gap between the read +// below and the unlink. An unlink by pathname alone would then remove that +// live replacement, not the dead gate this call inspected. A device-and- +// inode check cannot guard against this: deleting a file and immediately +// creating a new one at the same path can reuse the just-freed inode +// number on some filesystems, so a replacement gate can carry the very +// identity the deleted one had. Instead, this function records the exact +// bytes of the gate it inspected — the holder pid plus the random +// per-acquisition token `acquireAcpxRuntimeSandboxRootGate` writes — and +// re-reads the path immediately before unlinking, removing it only when +// the bytes still match exactly. A collision between two gates' random +// tokens is not a practical concern. async function breakStaleAcpxRuntimeSandboxRootGate( gatePath: string, + dependencies: AcpxRuntimeSandboxRootGateDependencies = {}, ): Promise { let holderPid: number | null = null; + let inspectedContents: string; try { const handle = await open( gatePath, @@ -795,7 +834,8 @@ async function breakStaleAcpxRuntimeSandboxRootGate( ); try { const bytes = await readFile(handle); - const parsed = Number.parseInt(bytes.toString("utf8").trim(), 10); + inspectedContents = bytes.toString("utf8").trim(); + const parsed = Number.parseInt(inspectedContents, 10); holderPid = Number.isInteger(parsed) && parsed > 0 ? parsed : null; } finally { await handle.close(); @@ -807,6 +847,20 @@ async function breakStaleAcpxRuntimeSandboxRootGate( if (holderPid !== null && isAcpxSandboxRootGateHolderAlive(holderPid)) { return; } + await dependencies.beforeStaleGateRemoval?.(); + let currentContents: string; + try { + currentContents = (await readFile(gatePath)).toString("utf8").trim(); + } catch (error) { + if (errorCode(error) === "ENOENT") return; + throw error; + } + if (currentContents !== inspectedContents) { + // The file at this path no longer carries the exact bytes this call + // inspected — another admission already replaced it. Leave the + // replacement alone. + return; + } await unlink(gatePath).catch((error) => { if (errorCode(error) !== "ENOENT") throw error; }); @@ -842,6 +896,7 @@ function reportDeclinedAcpxSandboxRootClaim(root: string): void { async function claimAcpxRuntimeSandboxRoot( root: string, + dependencies: AcpxRuntimeSandboxRootGateDependencies = {}, ): Promise { const entry = await lstat(root, { bigint: true }); if (entry.isSymbolicLink() || !entry.isDirectory()) { @@ -859,17 +914,22 @@ async function claimAcpxRuntimeSandboxRoot( // 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; - }); + 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; + }, + dependencies, + ); if (!claimed) { // Another admission already owns this deterministic root. Continue // without delete authority: never overwrite its marker, never touch the