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 2824392dafa..7baf6dc77c5 100644 --- a/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.test.ts +++ b/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.test.ts @@ -903,25 +903,104 @@ describe("ACPX runtime sandbox", () => { ); }); + // Free the third claimant's gate, as its own holder would on completing + // its claim, so the still-retrying recovery call above can stop + // retrying and acquire the now-free gate. Doing this before inspecting + // the stray capture avoids racing that inspection against the retry + // loop's own short-lived pin-and-unpin of the (still alive) third + // claimant's gate. + await unlink(gatePath); + // 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); + // where the capturing rename first put it. Each retry the loop already + // made against the third claimant's gate pinned and unpinned its own + // short-lived capture, so this polls until exactly the one expected + // stray settles rather than assuming a single snapshot lands cleanly + // between those. + const strandedCaptures = await waitForAcpxSandboxGateState(async () => { + const entries = (await readdir(dirname(gatePath))).filter((entry) => + entry.includes(".reap-"), + ); + expect(entries).toHaveLength(1); + return entries; + }); 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); }); + + it("never restores a captured gate once its own holder has already reclaimed it", 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 captures a live replacement gate, exactly like the "never + // clobbers a third claimant" scenario above. This time, before recovery + // decides whether to restore what it caught, the replacement's own + // holder finishes its critical section. A real holder's release finds + // nothing left at the shared path (this capture already moved it away) + // and reclaims its own capture directly by content instead. Recovery + // must then leave the shared path empty rather than restore a gate for + // a holder that is already done — a restored gate would still name this + // live test process, so every later admission and teardown for this + // root would read it as live and repeatedly reach the acquire timeout + // until this process exits. + 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" }); + }, + afterStaleGateCapture: async () => { + const directory = dirname(gatePath); + const entries = await readdir(directory); + let reclaimed = false; + for (const entry of entries) { + if (!entry.startsWith(`${basename(gatePath)}.reap-`)) continue; + const capturePath = join(directory, entry); + const contents = await readFile(capturePath, "utf8"); + if (contents !== replacementGateContents) continue; + await unlink(capturePath); + reclaimed = true; + } + expect(reclaimed).toBe(true); + }, + }, + ); + + // Recovery's restore attempt now finds nothing to restore, so the + // shared path settles empty and a fresh acquisition succeeds directly — + // never waiting out the replacement holder's own still-live process. + const sandbox = await recovered; + expect(sandbox.root).toBe(probe.root); + + const strandedCaptures = (await readdir(dirname(gatePath))).filter( + (entry) => entry.includes(".reap-"), + ); + expect(strandedCaptures).toHaveLength(0); + + 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 17694c303aa..a23625bf89f 100644 --- a/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.ts +++ b/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.ts @@ -770,13 +770,12 @@ async function withAcpxRuntimeSandboxRootGate( const gatePath = acpxRuntimeSandboxRootGatePath(root); const deadline = Date.now() + ACPX_SANDBOX_ROOT_GATE_ACQUIRE_TIMEOUT_MS; for (;;) { - if (await acquireAcpxRuntimeSandboxRootGate(gatePath)) { + const ownContent = await acquireAcpxRuntimeSandboxRootGate(gatePath); + if (ownContent !== null) { try { return await criticalSection(); } finally { - await unlink(gatePath).catch((error) => { - if (errorCode(error) !== "ENOENT") throw error; - }); + await releaseAcpxRuntimeSandboxRootGate(gatePath, ownContent); } } // The gate is held by another admission. Break it only when that holder @@ -792,9 +791,12 @@ async function withAcpxRuntimeSandboxRootGate( } } +// Returns this holder's own gate content on success, so its later release +// can identify its own gate by content rather than by trusting `gatePath` +// still names it (see `releaseAcpxRuntimeSandboxRootGate`). async function acquireAcpxRuntimeSandboxRootGate( gatePath: string, -): Promise { +): Promise { let handle: FileHandle; try { handle = await open( @@ -806,27 +808,86 @@ async function acquireAcpxRuntimeSandboxRootGate( PRIVATE_FILE_MODE, ); } catch (error) { - if (errorCode(error) === "EEXIST") return false; + if (errorCode(error) === "EEXIST") return null; throw error; } + // 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. + const content = `${process.pid}:${randomBytes(16).toString("hex")}`; try { await handle.chmod(PRIVATE_FILE_MODE); - // 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.writeFile(content, "utf8"); await handle.sync(); } finally { await handle.close(); } - return true; + return content; +} + +// Releases this holder's own gate. A plain unlink by pathname is usually +// enough, but a concurrent stale-gate recovery can have captured this exact +// gate under a private reap name for inspection (see +// `breakStaleAcpxRuntimeSandboxRootGate`) just before this call runs, +// leaving nothing at `gatePath` to unlink even though this holder's +// critical section has genuinely ended. Left alone, that recovery can then +// restore what it captured after this holder is already done, resurrecting +// a gate nobody holds — later admissions would read its still-alive pid and +// treat it as live indefinitely. When the plain unlink finds nothing, this +// call instead finds and removes the capture directly, by content rather +// than by guessing its private name, so a recovery that later tries to +// restore it finds nothing there. A capture can also already have been +// restored back to `gatePath` by the time this call notices the first +// unlink failed, so this call retries the plain unlink once more after +// clearing any capture, closing the gap in either ordering. +async function releaseAcpxRuntimeSandboxRootGate( + gatePath: string, + ownContent: string, +): Promise { + const releasedDirectly = await unlink(gatePath) + .then(() => true) + .catch((error) => { + if (errorCode(error) === "ENOENT") return false; + throw error; + }); + if (releasedDirectly) return; + await reclaimAcpxRuntimeSandboxRootGateCapture(gatePath, ownContent); + await unlink(gatePath).catch((error) => { + if (errorCode(error) !== "ENOENT") throw error; + }); +} + +// Finds and removes any reap-named capture of `gatePath` whose content +// matches `ownContent`, so a stale-gate recovery holding that exact capture +// cannot restore it after this holder has already moved on. +async function reclaimAcpxRuntimeSandboxRootGateCapture( + gatePath: string, + ownContent: string, +): Promise { + const directory = dirname(gatePath); + const prefix = `${basename(gatePath)}${ACPX_SANDBOX_ROOT_GATE_REAP_INFIX}`; + let entries: string[]; + try { + entries = await readdir(directory); + } catch (error) { + if (errorCode(error) === "ENOENT") return; + throw error; + } + for (const entry of entries) { + if (!entry.startsWith(prefix)) continue; + const capturePath = join(directory, entry); + const contents = await readFile(capturePath, "utf8").catch((error) => { + if (errorCode(error) === "ENOENT") return null; + throw error; + }); + if (contents !== ownContent) continue; + await unlink(capturePath).catch((error) => { + if (errorCode(error) !== "ENOENT") throw error; + }); + } } // Removes a gate file left behind by a holder process that no longer exists, @@ -969,12 +1030,26 @@ async function removeStaleAcpxRuntimeSandboxRootGate( 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. + // path after this call proved the original dead: put it straight back, + // unless that claimant's own critical section has since ended. Restoring + // a gate after its holder is done would resurrect it: the holder's own + // release finds nothing at `gatePath` to unlink (this rename already + // moved it away), so it reclaims `removalPath` directly by content + // instead (see `releaseAcpxRuntimeSandboxRootGate`). A `link` failing + // with ENOENT here means that reclaim already won this race, so there is + // nothing left to restore. await dependencies.afterStaleGateCapture?.(); try { await link(removalPath, gatePath); } catch (error) { - if (errorCode(error) !== "EEXIST") throw error; + const code = errorCode(error); + if (code === "ENOENT") { + // The claimant already finished and reclaimed its own gate out from + // under this capture: its critical section is over, so leaving the + // shared path empty is correct, not a leak. + return; + } + if (code !== "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