From 56043bf2e49788c741266238e6945ae18579e96e Mon Sep 17 00:00:00 2001 From: Priya Raman Date: Thu, 3 Sep 2026 14:21:15 +0000 Subject: [PATCH] fix(runner): stop a displaced ACPX gate holder's release from deleting a fresh claimant's gate Stale-gate recovery can capture a genuinely live gate and fail to restore it, when a fresh claimant grabs the shared path first. The displaced holder's own release then found something at the shared path and unlinked it without checking whose gate it was, deleting the fresh claimant's live gate. That broke the fresh claimant's exclusivity and let a teardown overlap a live lease registration. Release now captures whatever names the shared path with the same atomic rename recovery uses, and only deletes it when its content proves it is this holder's own gate. A mismatch is restored, or, if a further claimant has since taken the path, left under a private name for its real owner to reclaim by content later. Co-authored-by: Paperclip --- .../src/drivers/acpx/runtime-sandbox.test.ts | 125 ++++++++++++++++ .../src/drivers/acpx/runtime-sandbox.ts | 134 +++++++++++++++--- 2 files changed, 236 insertions(+), 23 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 e9b55408a9f..dfa8f60010e 100644 --- a/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.test.ts +++ b/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.test.ts @@ -1060,6 +1060,131 @@ describe("ACPX runtime sandbox", () => { await recoveringProcess.removeOwnedAcpxRuntimeSandboxRoot(sandbox); }); + + it("never lets a displaced holder's own release clobber the gate a third claimant put in its place", 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"); + + let resumeDisplacedHolder: () => void = () => {}; + const displacedHolderPaused = new Promise((settle) => { + resumeDisplacedHolder = settle; + }); + let displacedHolderProcess: typeof import("./runtime-sandbox.js") | null = + null; + let displacedHolderClaim: ReturnType | null = + null; + const thirdClaimantGateContents = `${process.pid}:third-claimant`; + + // Recovery proves the dead gate stale, then, before it decides whether + // the shared path still names that exact dead gate, a real second + // admission wins the shared path with its own live gate and holds its + // critical section open right up to its own release — the moment a + // stale-gate recovery pass can displace a genuinely live holder. + const recovered = recoveringProcess.prepareAcpxRuntimeSandbox( + { binding: fixture.binding, agent: "codex" }, + { + beforeStaleGateRemoval: async () => { + await unlink(gatePath); + + vi.resetModules(); + displacedHolderProcess = await import("./runtime-sandbox.js"); + displacedHolderClaim = displacedHolderProcess.prepareAcpxRuntimeSandbox( + { binding: fixture.binding, agent: "codex" }, + { beforeGateRelease: () => displacedHolderPaused }, + ); + + // Wait until the displaced holder has published its own real gate + // and paused just before releasing it, so recovery's capture below + // catches a genuinely complete live gate, never an in-progress + // write. + await waitForAcpxSandboxGateState(async () => { + const contents = await readFile(gatePath, "utf8").catch( + () => null, + ); + expect(contents).toMatch(/^\d+:[0-9a-f]{32}$/); + }); + }, + afterStaleGateCapture: async () => { + // A third claimant grabs the shared path in the instant recovery's + // capture vacates it, before recovery's own restore can land. + 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 displaced holder's own live gate was not lost: it is stranded + // under a private reap name, off the shared path, because recovery's + // restore lost the shared path to the third claimant. + const strandedBeforeRelease = await waitForAcpxSandboxGateState( + async () => { + const entries = (await readdir(dirname(gatePath))).filter((entry) => + entry.includes(".reap-"), + ); + expect(entries).toHaveLength(1); + return entries; + }, + ); + const displacedHolderGateContents = await readFile( + join(dirname(gatePath), strandedBeforeRelease[0]), + "utf8", + ); + expect(displacedHolderGateContents).toMatch(/^\d+:[0-9a-f]{32}$/); + + // Let the displaced holder proceed to release its own gate. Its release + // must find its own gate under the stray reap name, not at the shared + // path, and must never touch the third claimant's gate now sitting + // there — the exact failure this test guards against. + resumeDisplacedHolder(); + const displacedHolderSandbox = await displacedHolderClaim!; + expect(displacedHolderSandbox.root).toBe(probe.root); + + await expect(readFile(gatePath, "utf8")).resolves.toBe( + thirdClaimantGateContents, + ); + expect( + (await readdir(dirname(gatePath))).filter((entry) => + entry.includes(".reap-"), + ), + ).toHaveLength(0); + + // 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. + await unlink(gatePath); + + const sandbox = await recovered; + expect(sandbox.root).toBe(probe.root); + + await displacedHolderProcess!.removeOwnedAcpxRuntimeSandboxRoot( + displacedHolderSandbox, + ); + 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 fac41fb4b98..2c468ae19d9 100644 --- a/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.ts +++ b/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.ts @@ -440,6 +440,15 @@ export interface AcpxRuntimeSandboxPrepareDependencies { * prove recovery's restore never clobbers that claim. */ afterStaleGateCapture?: () => Promise; + /** + * Internal test seam. Runs once this admission's own critical section has + * ended, immediately before it releases its gate. Lets a test hold a + * still-live gate open at the shared path, to prove a concurrent + * stale-gate recovery pass can displace it without this admission's own + * later release ever touching whatever a fresh claimant has since put in + * its place. + */ + beforeGateRelease?: () => Promise; } /** Prepare the private filesystem and environment visible to an ACPX agent. */ @@ -770,6 +779,13 @@ interface AcpxRuntimeSandboxRootGateDependencies { * atomic capture and the restore in `removeStaleAcpxRuntimeSandboxRootGate`, * exercised only in tests. */ afterStaleGateCapture?: () => Promise; + /** Internal seam that runs once this holder's own critical section has + * ended, immediately before it releases its gate. Lets a test hold a + * still-live gate open at the shared path, to prove a concurrent stale-gate + * recovery pass can displace it without this holder's own later release + * ever touching whatever a fresh claimant has since put in its place. + * Exercised only in tests. */ + beforeGateRelease?: () => Promise; } /** @@ -796,6 +812,7 @@ async function withAcpxRuntimeSandboxRootGate( try { return await criticalSection(); } finally { + await dependencies.beforeGateRelease?.(); await releaseAcpxRuntimeSandboxRootGate(gatePath, ownContent); } } @@ -874,36 +891,107 @@ async function acquireAcpxRuntimeSandboxRootGate( 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. +// Releases this holder's own gate. This must remove only the exact gate +// instance this holder acquired, never a fresh gate a later claimant has +// since put at the shared path — so a plain, unconditional `unlink(gatePath)` +// is never safe here. A concurrent stale-gate recovery pass can displace +// this holder's own still-live gate before this call runs (see +// `removeStaleAcpxRuntimeSandboxRootGate`): it captures whatever names the +// shared path with an atomic `rename`, and when a fresh claimant grabs the +// now-momentarily-empty shared path before recovery's restore can land, +// recovery leaves the captured gate stranded under a private name and +// never touches the shared path again. From this holder's point of view, +// by the time its own critical section ends, `gatePath` can therefore name +// three different things: this holder's own gate (the common case, nothing +// displaced it), nothing at all (recovery captured it and is still +// deciding what to do), or a different claimant's gate entirely (recovery +// captured this holder's gate and a fresh claimant has since taken the +// shared path). An unconditional unlink cannot tell these apart, so it +// would delete a later claimant's live gate right along with its own, +// reopening the exact claim-versus-teardown race this gate exists to +// close. +// +// This call instead captures whatever currently names `gatePath` with the +// same atomic `rename` recovery uses, then only proceeds by content: a +// match proves it caught its own gate, safe to delete for good; a mismatch +// means some other claimant's gate was caught by mistake, so this call +// restores it immediately with `link` — never `rename` — so a third +// claimant that has since taken the now-momentarily-empty shared path is +// never clobbered by the restore. Either way, once the shared path is no +// longer this holder's own concern, this call finishes by searching for its +// own gate under a stray reap-named capture with +// `reclaimAcpxRuntimeSandboxRootGateCapture`, since stale-gate recovery may +// still be holding it there, or may have already restored it back to +// `gatePath` in between, in which case a second capture attempt finds and +// removes it directly. 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; + if ( + (await captureAndReleaseAcpxRuntimeSandboxRootGate( + gatePath, + ownContent, + )) === "released" + ) { + return; + } await reclaimAcpxRuntimeSandboxRootGateCapture(gatePath, ownContent); - await unlink(gatePath).catch((error) => { + await captureAndReleaseAcpxRuntimeSandboxRootGate(gatePath, ownContent); +} + +type AcpxRuntimeSandboxRootGateCaptureOutcome = + | "released" + | "absent" + | "displaced"; + +// Atomically captures whatever currently names `gatePath` and, only when its +// content proves it is this holder's own gate, deletes it for good. +// Whatever else this call finds at `gatePath` — a different claimant's live +// gate, or nothing at all — is left exactly as this holder found it: a +// mismatched capture is put straight back (or, if a fresh claimant has since +// taken the shared path, left under its own private name for that gate's +// real owner to find later by content), and a missing gate means there is +// nothing here for this holder to release directly. +async function captureAndReleaseAcpxRuntimeSandboxRootGate( + gatePath: string, + ownContent: string, +): Promise { + const capturePath = `${gatePath}${ACPX_SANDBOX_ROOT_GATE_REAP_INFIX}${process.pid}-${randomBytes(8).toString("hex")}`; + try { + await rename(gatePath, capturePath); + } catch (error) { + // Nothing at the shared path to release directly. + if (errorCode(error) === "ENOENT") return "absent"; + throw error; + } + const contents = await readFile(capturePath, "utf8").catch((error) => { + if (errorCode(error) === "ENOENT") return null; + throw error; + }); + if (contents === ownContent) { + await unlink(capturePath).catch((error) => { + if (errorCode(error) !== "ENOENT") throw error; + }); + return "released"; + } + // The rename caught a gate that is not this holder's own: a different + // claimant's gate was at the shared path. Put it straight back, unless a + // further claimant has since taken the now-momentarily-empty shared path. + try { + await link(capturePath, gatePath); + } catch (error) { + if (errorCode(error) !== "EEXIST") throw error; + // A further claimant grabbed the shared path first: leave the captured + // gate under its own private name rather than clobber that claim. Its + // real owner's own release finds it later, by content, the same way + // this call looks for its own gate below. + return "displaced"; + } + await unlink(capturePath).catch((error) => { if (errorCode(error) !== "ENOENT") throw error; }); + return "displaced"; } // Finds and removes any reap-named capture of `gatePath` whose content