From c83169a20ee490d0bc018c9009fbce8686927c10 Mon Sep 17 00:00:00 2001 From: Priya Raman Date: Thu, 3 Sep 2026 15:28:48 +0000 Subject: [PATCH] fix(runner): serialize ACPX stale-gate recovery so two admissions never race the same dead gate Two admissions could independently prove the same crashed holder's gate stale and both proceed to remove it. The first removal vacates the shared path and deletes the dead gate; a fresh claimant can win that vacant path before the second admission's own removal runs. That second removal then captures the fresh claimant's live gate by mistake, forcing it through a vacate-then-restore dance that opens a further window for a claimant to overlap its critical section with the live holder recovery just displaced. Serialize recovery for one gate across every Runner process with a single well-known lock path, so at most one admission is ever mid-recovery for a given gate. Its own removal step then always captures the exact dead gate it already proved stale, so the vacate-then-restore case becomes unreachable in production. A stale lock (its holder crashed mid-recovery) is cleared by age, so a crash cannot block every later admission for the root forever. Co-authored-by: Paperclip --- .../src/drivers/acpx/runtime-sandbox.test.ts | 128 ++++++++++++++++++ .../src/drivers/acpx/runtime-sandbox.ts | 127 ++++++++++++++++- 2 files changed, 253 insertions(+), 2 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 dfa8f60010e..56a094f546e 100644 --- a/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.test.ts +++ b/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.test.ts @@ -1185,6 +1185,134 @@ describe("ACPX runtime sandbox", () => { ); await recoveringProcess.removeOwnedAcpxRuntimeSandboxRoot(sandbox); }); + + it("serializes concurrent stale-gate recovery so a second attempt never races its own capture of the same gate", 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`); + const recoveryLockPath = `${gatePath}.recovering`; + + // 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"]); + const deadHolderContents = String(deadHolder.pid); + await writeFile(gatePath, deadHolderContents, { flag: "wx" }); + + vi.resetModules(); + const firstRecoveringProcess: typeof import("./runtime-sandbox.js") = + await import("./runtime-sandbox.js"); + let releaseFirstRecovery: () => void = () => {}; + const firstRecoveryPaused = new Promise((settle) => { + releaseFirstRecovery = settle; + }); + const firstRecovered = firstRecoveringProcess.prepareAcpxRuntimeSandbox( + { binding: fixture.binding, agent: "codex" }, + { afterRecoveryLockAcquired: () => firstRecoveryPaused }, + ); + + // Wait until the first attempt has won the recovery lock, so the second + // attempt below is guaranteed to find it already held. + await waitForAcpxSandboxGateState(async () => { + await expect(stat(recoveryLockPath)).resolves.toBeDefined(); + }); + + // A second admission also detects the same dead gate and tries to + // recover it while the first attempt still holds the recovery lock. + vi.resetModules(); + const secondRecoveringProcess: typeof import("./runtime-sandbox.js") = + await import("./runtime-sandbox.js"); + const secondRecovered = secondRecoveringProcess.prepareAcpxRuntimeSandbox( + { binding: fixture.binding, agent: "codex" }, + ); + await waitForConcurrentClaimHeadStart(); + + // The second attempt backs off without ever pinning the gate: no stray + // capture appears, and the dead gate itself stays exactly as it was. + // Only the first attempt's own recovery pass ever touches this gate. + await expect(readFile(gatePath, "utf8")).resolves.toBe( + deadHolderContents, + ); + expect( + (await readdir(dirname(gatePath))).filter((entry) => + entry.includes(".reap-"), + ), + ).toHaveLength(0); + + releaseFirstRecovery(); + const [firstSandbox, secondSandbox] = await Promise.all([ + firstRecovered, + secondRecovered, + ]); + expect(firstSandbox.root).toBe(probe.root); + expect(secondSandbox.root).toBe(probe.root); + + await firstRecoveringProcess.removeOwnedAcpxRuntimeSandboxRoot( + firstSandbox, + ); + await secondRecoveringProcess.removeOwnedAcpxRuntimeSandboxRoot( + secondSandbox, + ); + }); + + it("clears a stale recovery lock left behind by a recovery attempt that crashed before it finished", 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`); + const recoveryLockPath = `${gatePath}.recovering`; + + // 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"]); + const deadHolderContents = String(deadHolder.pid); + await writeFile(gatePath, deadHolderContents, { flag: "wx" }); + + // A different process crashed mid-recovery, after it won the recovery + // lock but before it ever released it: the lock file is left behind at + // the shared path. + await writeFile(recoveryLockPath, "crashed-recoverer", { flag: "wx" }); + + vi.resetModules(); + const waitingProcess: typeof import("./runtime-sandbox.js") = + await import("./runtime-sandbox.js"); + const waiting = waitingProcess.prepareAcpxRuntimeSandbox({ + binding: fixture.binding, + agent: "codex", + }); + await waitForConcurrentClaimHeadStart(); + + // The lock is not cleared this soon: recovery cannot yet prove its + // holder crashed rather than merely still being mid-recovery. + await expect(stat(recoveryLockPath)).resolves.toBeDefined(); + await expect(readFile(gatePath, "utf8")).resolves.toBe( + deadHolderContents, + ); + + // Move the clock forward past the recovery lock's own stale threshold, + // the same way real elapsed time would, without a real multi-second + // wait. + const dateNowSpy = vi + .spyOn(Date, "now") + .mockReturnValue(Date.now() + 5_500); + try { + const sandbox = await waiting; + expect(sandbox.root).toBe(probe.root); + await expect(stat(recoveryLockPath)).rejects.toMatchObject({ + code: "ENOENT", + }); + await expect(stat(gatePath)).rejects.toMatchObject({ code: "ENOENT" }); + await waitingProcess.removeOwnedAcpxRuntimeSandboxRoot(sandbox); + } finally { + dateNowSpy.mockRestore(); + } + }); }); 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 2c468ae19d9..9fadea3218e 100644 --- a/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.ts +++ b/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.ts @@ -130,6 +130,16 @@ const ACPX_SANDBOX_ROOT_GATE_STAGE_INFIX = ".stage-"; // `ACPX_SANDBOX_ROOT_GATE_ACQUIRE_TIMEOUT_MS`, so a waiting admission gets // more than one chance to recover the gate before it gives up. const ACPX_SANDBOX_ROOT_GATE_INIT_GRACE_MS = 1_000; +// A single well-known path (not a random one) for the lock that serializes +// stale-gate recovery attempts for one gate across every Runner process. See +// `withAcpxRuntimeSandboxRootGateRecoveryLock`. +const ACPX_SANDBOX_ROOT_GATE_RECOVERY_LOCK_SUFFIX = ".recovering"; +// One recovery attempt only runs local filesystem calls, never blocking I/O +// on another process, so it always finishes well inside this bound. A lock +// still held past it can only mean its own holder crashed mid-recovery; a +// later attempt then clears it, instead of leaving every admission for this +// root to wait out the full gate acquire timeout. +const ACPX_SANDBOX_ROOT_GATE_RECOVERY_LOCK_STALE_MS = 5_000; export interface AcpxRuntimeSandbox { root: string; @@ -449,6 +459,13 @@ export interface AcpxRuntimeSandboxPrepareDependencies { * its place. */ beforeGateRelease?: () => Promise; + /** + * Internal test seam. Runs once this admission has won the stale-gate + * recovery lock, immediately before it inspects the gate. Lets a test hold + * the lock open and prove a second, concurrent recovery attempt backs off + * instead of racing its own capture of the same gate. + */ + afterRecoveryLockAcquired?: () => Promise; } /** Prepare the private filesystem and environment visible to an ACPX agent. */ @@ -786,6 +803,12 @@ interface AcpxRuntimeSandboxRootGateDependencies { * ever touching whatever a fresh claimant has since put in its place. * Exercised only in tests. */ beforeGateRelease?: () => Promise; + /** Internal seam that runs once this admission has won the stale-gate + * recovery lock, immediately before it inspects the gate. Lets a test hold + * the lock open and prove a second, concurrent recovery attempt backs off + * instead of racing its own capture of the same gate. Exercised only in + * tests. */ + afterRecoveryLockAcquired?: () => Promise; } /** @@ -818,8 +841,13 @@ 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, dependencies); + // slow-but-live holder is never pre-empted. Run the break itself under + // the recovery lock, so at most one admission ever inspects and removes + // a given dead gate at a time — see + // `withAcpxRuntimeSandboxRootGateRecoveryLock`. + await withAcpxRuntimeSandboxRootGateRecoveryLock(gatePath, dependencies, () => + breakStaleAcpxRuntimeSandboxRootGate(gatePath, dependencies), + ); if (Date.now() >= deadline) { throw new Error( "ACPX sandbox root ownership gate could not be acquired", @@ -1024,6 +1052,101 @@ async function reclaimAcpxRuntimeSandboxRootGateCapture( } } +// Serializes stale-gate recovery attempts for one gate across every Runner +// process, using a single well-known lock path (not a random one, unlike +// every other private path this file creates), so at most one admission +// ever runs `breakStaleAcpxRuntimeSandboxRootGate` for a given gate at a +// time. +// +// Without this lock, two admissions can independently pin the same dead +// gate, both prove its holder dead, and both proceed to remove it. The +// first admission's removal atomically vacates the shared path and deletes +// the dead gate for good. In the instant the shared path is vacant, a fresh +// claimant can win it with its own live gate — before the SECOND admission +// even runs its own removal. That second admission's removal then captures +// the fresh claimant's live gate instead of the dead one it pinned, so it +// must vacate the shared path again to inspect what it caught, and restore +// it once it proves the capture is not its own. That second vacate-then +// -restore step is itself a fresh window: a further claimant can win the +// shared path inside it and start running its own critical section fully +// concurrently with the live holder recovery just displaced, defeating the +// exclusivity this whole gate exists to give. +// +// Serializing recovery closes this at the source: only one admission is +// ever mid-recovery for a given gate, so its own removal step is always +// the very first thing to touch the shared path since it proved that gate +// stale. Nothing else can have replaced the gate in between, so its removal +// always captures the exact dead gate it pinned, and the vacate-then-restore +// case this lock protects against becomes unreachable in production — kept +// here only as a defensive fallback for a change that has not proved this +// invariant. +// +// The lock itself only guards a short run of local filesystem calls, never +// blocking I/O on another process, so a live lock holder always finishes +// well inside `ACPX_SANDBOX_ROOT_GATE_RECOVERY_LOCK_STALE_MS`. A lock still +// held past that age can only mean its own holder crashed mid-recovery; +// this call then clears it so a later attempt is not blocked forever by a +// lock its own holder can never release. +async function withAcpxRuntimeSandboxRootGateRecoveryLock( + gatePath: string, + dependencies: AcpxRuntimeSandboxRootGateDependencies, + recovery: () => Promise, +): Promise { + const lockPath = `${gatePath}${ACPX_SANDBOX_ROOT_GATE_RECOVERY_LOCK_SUFFIX}`; + let handle: FileHandle; + try { + handle = await open( + lockPath, + constants.O_WRONLY | + constants.O_CREAT | + constants.O_EXCL | + (constants.O_NOFOLLOW ?? 0), + PRIVATE_FILE_MODE, + ); + } catch (error) { + if (errorCode(error) !== "EEXIST") throw error; + // Another admission is already recovering this gate. Clear the lock + // only once it is old enough that its own holder must have crashed + // mid-recovery, and either way, back off for this call: attempting the + // gate's own recovery here too is exactly the race this lock exists to + // prevent. A stale lock cleared here lets the NEXT retry in + // `withAcpxRuntimeSandboxRootGate`'s loop acquire it cleanly, instead of + // this call racing a second recovery attempt into the same gate. + const entry = await stat(lockPath).catch((statError) => { + if (errorCode(statError) === "ENOENT") return null; + throw statError; + }); + if ( + entry !== null && + Date.now() - entry.mtimeMs >= ACPX_SANDBOX_ROOT_GATE_RECOVERY_LOCK_STALE_MS + ) { + await unlink(lockPath).catch((unlinkError) => { + if (errorCode(unlinkError) !== "ENOENT") throw unlinkError; + }); + } + return; + } + // The lock's own existence, at this one well-known path, is the whole + // exclusivity primitive: nothing here ever inspects its content, only its + // presence and its age, so — like the lease files beside it — it carries + // no content and is never synced. Losing an un-synced create on a crash + // only means the lock looks absent right after a reboot, the same as a + // clean release; it never causes a live lock to look absent. + try { + await handle.chmod(PRIVATE_FILE_MODE); + } finally { + await handle.close(); + } + try { + await dependencies.afterRecoveryLockAcquired?.(); + await recovery(); + } finally { + await unlink(lockPath).catch((error) => { + if (errorCode(error) !== "ENOENT") throw error; + }); + } +} + // Removes a gate file left behind by a holder process that no longer exists, // so a hard process crash inside the critical section cannot block every // later admission for this root forever. Never removes a gate whose holder