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