From 653d19732cd57127753a5d2ed21cb57bd2463cab Mon Sep 17 00:00:00 2001 From: Priya Raman Date: Thu, 3 Sep 2026 13:37:47 +0000 Subject: [PATCH] fix(runner): stop stale ACPX gate recovery from restoring a gate after its holder finishes Recovery can capture a live replacement gate for inspection, then find its holder already done by the time it decides to restore it. Restoring it anyway resurrected a gate for a process that had moved on: since the process itself stays alive, every later admission or teardown for that root read the gate as live and hit the acquire timeout, with no recovery path short of the process exiting. A holder's own release now finds and removes any capture of its own gate by content when a plain unlink finds nothing at the shared path, and recovery's restore now treats a vanished capture as nothing left to restore, instead of throwing. Also makes the "third claimant" regression test poll for its expected stray count instead of taking one snapshot, since the retry loop's own short-lived pin-and-unpin cycles against a still-alive gate could otherwise land inside that snapshot on a loaded CI runner. Co-authored-by: Paperclip --- .../src/drivers/acpx/runtime-sandbox.test.ts | 97 +++++++++++++-- .../src/drivers/acpx/runtime-sandbox.ts | 115 +++++++++++++++--- 2 files changed, 183 insertions(+), 29 deletions(-) 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