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 2fe72c20b4e..3f90b8045f5 100644 --- a/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.test.ts +++ b/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.test.ts @@ -1464,11 +1464,11 @@ describe("ACPX runtime sandbox", () => { // The second admission's own fresh recovery lock is now live at the // shared path, created only after it broke the crashed recoverer's - // stale one for real. A real lock carries no content — like the lease - // files beside it, only its presence and its age ever matter — so its - // identity, not its content, is what proves whether it survives - // untouched: an admission that deletes it and creates its own new one - // in its place would look identical by content alone. + // stale one for real. This check asserts on the lock's identity, not + // its content: what follows never deletes and recreates this exact + // lock, only moves it aside and links it straight back, so its device + // and inode stay stable throughout and prove whether it survives + // untouched. const freshLockIdentity = await waitForAcpxSandboxGateState(async () => { const contents = await readFile(recoveryLockPath, "utf8").catch( () => null, @@ -1516,6 +1516,105 @@ describe("ACPX runtime sandbox", () => { await secondProcess.removeOwnedAcpxRuntimeSandboxRoot(secondSandbox); await firstProcess.removeOwnedAcpxRuntimeSandboxRoot(firstSandbox); }); + + it("never removes a replacement recovery lock after a live recovery's own lock is wrongly broken as stale", 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 firstProcess: typeof import("./runtime-sandbox.js") = + await import("./runtime-sandbox.js"); + let releaseFirstLock: () => void = () => {}; + const firstLockPaused = new Promise((settle) => { + releaseFirstLock = settle; + }); + const firstRecovered = firstProcess.prepareAcpxRuntimeSandbox( + { binding: fixture.binding, agent: "codex" }, + { afterRecoveryLockAcquired: () => firstLockPaused }, + ); + + // Wait until the first admission has won a genuinely live recovery + // lock, before its own recovery pass ever runs. + await waitForAcpxSandboxGateState(async () => { + await expect(stat(recoveryLockPath)).resolves.toBeDefined(); + }); + + // A second admission finds the same dead gate and the first admission's + // lock in its way. Move the clock forward past the recovery lock's own + // stale threshold before the second admission judges the lock's age, so + // it wrongly treats the first admission's still-live lock as abandoned + // by a crashed recoverer — the first admission has done nothing wrong; + // the lock is simply old enough by the mocked clock to look that way. + const dateNowSpy = vi + .spyOn(Date, "now") + .mockReturnValue(Date.now() + 5_500); + vi.resetModules(); + const secondProcess: typeof import("./runtime-sandbox.js") = + await import("./runtime-sandbox.js"); + let releaseSecondLock: () => void = () => {}; + const secondLockPaused = new Promise((settle) => { + releaseSecondLock = settle; + }); + // The second admission's first attempt at the lock is guaranteed to hit + // the first admission's still-present lock and go through a break + // attempt, since the first admission never releases before this signal + // fires — so reaching this seam again proves the second admission's own + // fresh acquisition, on its next retry, not a first-attempt success. + let markSecondLockAcquired: () => void = () => {}; + const secondLockAcquired = new Promise((settle) => { + markSecondLockAcquired = settle; + }); + const secondRecovered = secondProcess.prepareAcpxRuntimeSandbox( + { binding: fixture.binding, agent: "codex" }, + { + afterRecoveryLockAcquired: () => { + markSecondLockAcquired(); + return secondLockPaused; + }, + }, + ); + + // The second admission breaks the first admission's real lock (it looks + // stale under the mocked clock) and, on its next retry, wins a fresh + // lock of its own at the same path — the replacement the first + // admission's own eventual release must not remove. + await secondLockAcquired; + dateNowSpy.mockRestore(); + + // The first admission resumes now, unaware its own lock was already + // broken. It must never remove the second admission's live replacement + // lock when its own recovery finishes and it releases what it believes + // is still its own lock. The second admission is still paused and has + // not released its own lock itself, so the lock still being present + // right after the first admission's own recovery finishes proves the + // first admission's release left it alone. + releaseFirstLock(); + const firstSandbox = await firstRecovered; + expect(firstSandbox.root).toBe(probe.root); + await expect(stat(recoveryLockPath)).resolves.toBeDefined(); + + // The second admission now proceeds with its own recovery pass, using + // its still-intact replacement lock, and can claim the same, now-freed + // root for itself too. + releaseSecondLock(); + const secondSandbox = await secondRecovered; + expect(secondSandbox.root).toBe(probe.root); + + await firstProcess.removeOwnedAcpxRuntimeSandboxRoot(firstSandbox); + await secondProcess.removeOwnedAcpxRuntimeSandboxRoot(secondSandbox); + }); }); 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 1701db1fe6c..aa8a7844de0 100644 --- a/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.ts +++ b/packages/paperclip-runner/src/drivers/acpx/runtime-sandbox.ts @@ -139,6 +139,14 @@ const ACPX_SANDBOX_ROOT_GATE_RECOVERY_LOCK_SUFFIX = ".recovering"; // 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; +// Infix for the private path a recovery lock holder pins its own lock +// instance under for as long as it holds the lock. Distinct from +// `ACPX_SANDBOX_ROOT_GATE_REAP_INFIX`, whose files are always short-lived +// (created and removed within a single function call): this pin instead +// lives for a holder's entire recovery pass, so a test or a directory scan +// that expects only transient reap files to ever appear must not mistake it +// for one. See `withAcpxRuntimeSandboxRootGateRecoveryLock`. +const ACPX_SANDBOX_ROOT_GATE_RECOVERY_LOCK_PIN_INFIX = ".holder-"; export interface AcpxRuntimeSandbox { root: string; @@ -1136,14 +1144,25 @@ async function withAcpxRuntimeSandboxRootGateRecoveryLock( ); 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. + // Pin this exact lock instance under a private path this holder alone + // controls, for as long as it holds the lock. A plain device-and-inode + // check taken now and compared again at release time would not be safe on + // its own: if a stale-lock break removes the shared name out from under + // this holder in between, the filesystem can reuse the just-freed inode + // number for an unrelated replacement lock (observed on this file's own + // tmp filesystem, the same hazard the gate's own identity checks already + // guard against), so a replacement could carry the very same identity the + // removed lock had. This extra link keeps that exact inode allocated for + // as long as the pin survives, so its device and inode can never be + // confused with a later, unrelated file's — the pin is this holder's own + // private name, so no break or release anywhere else in this file ever + // touches it, and only this holder's own release removes it. + const ownPinPath = `${lockPath}${ACPX_SANDBOX_ROOT_GATE_RECOVERY_LOCK_PIN_INFIX}${process.pid}-${randomBytes(8).toString("hex")}`; + let ownEntry: BigIntStats; try { await handle.chmod(PRIVATE_FILE_MODE); + await link(lockPath, ownPinPath); + ownEntry = await lstat(ownPinPath, { bigint: true }); } finally { await handle.close(); } @@ -1151,7 +1170,92 @@ async function withAcpxRuntimeSandboxRootGateRecoveryLock( await dependencies.afterRecoveryLockAcquired?.(); await recovery(); } finally { - await unlink(lockPath).catch((error) => { + await releaseAcpxRuntimeSandboxRootGateRecoveryLock( + lockPath, + ownPinPath, + ownEntry, + ); + } +} + +// Releases this holder's own recovery lock. This must remove only the exact +// lock instance this holder acquired, never a replacement a later admission +// has since acquired at the same path — so a plain, unconditional +// `unlink(lockPath)` is never safe here. `breakStaleAcpxRuntimeSandboxRootGateRecoveryLock` +// can judge this holder's own still-live lock stale (its own stale threshold +// is a heuristic, not a proof of death) and remove it while this holder is +// still genuinely mid-recovery; a later admission can then win a fresh lock +// of its own at the now-vacant path before this holder's recovery finishes. +// From this holder's point of view, by the time it releases, `lockPath` can +// therefore name either its own lock (the common case) or a different +// admission's live replacement — an unconditional unlink cannot tell these +// apart, and would delete that replacement right along with its own, +// letting a third admission win a fresh lock and run its own recovery +// concurrently with whichever admission the replacement belongs to. +// +// This call instead captures whatever currently names `lockPath` with the +// same atomic `rename` stale-lock recovery uses, then proceeds only by the +// identity `ownPinPath` still pins: a match proves this call caught its own +// lock, safe to delete for good; a mismatch means a different admission's +// live lock was caught by mistake, so this call restores it immediately +// with `link`, not `rename`, so a further admission that has since taken +// the now-momentarily-empty shared path is never clobbered by the restore. +// Either way, this holder's own pin is no longer needed once the shared +// path is no longer its concern, so this call always releases it last. +async function releaseAcpxRuntimeSandboxRootGateRecoveryLock( + lockPath: string, + ownPinPath: string, + ownEntry: BigIntStats, +): Promise { + try { + const capturePath = `${lockPath}${ACPX_SANDBOX_ROOT_GATE_REAP_INFIX}${process.pid}-${randomBytes(8).toString("hex")}`; + try { + await rename(lockPath, capturePath); + } catch (error) { + // Nothing at the shared path to release directly. + if (errorCode(error) === "ENOENT") return; + throw error; + } + const capturedEntry = await lstat(capturePath, { bigint: true }).catch( + (error) => { + if (errorCode(error) === "ENOENT") return null; + throw error; + }, + ); + if (capturedEntry === null) { + // The capture is gone already: `capturePath` is a private path only + // this call knows, so nothing else should ever remove it, but this + // is defensive against that regardless. + return; + } + if ( + capturedEntry.dev === ownEntry.dev && + capturedEntry.ino === ownEntry.ino + ) { + await unlink(capturePath).catch((error) => { + if (errorCode(error) !== "ENOENT") throw error; + }); + return; + } + // The rename caught a lock that is not this holder's own: a different + // admission's live lock was at the shared path. Put it straight back, + // unless a further admission has since taken the now-momentarily-empty + // shared path. + try { + await link(capturePath, lockPath); + } catch (error) { + if (errorCode(error) !== "EEXIST") throw error; + // A further admission grabbed the shared path first: leave the + // captured lock under its own private name rather than clobber that + // claim. Its real owner's own release finds it later, the same way + // this call would have found its own lock above. + return; + } + await unlink(capturePath).catch((error) => { + if (errorCode(error) !== "ENOENT") throw error; + }); + } finally { + await unlink(ownPinPath).catch((error) => { if (errorCode(error) !== "ENOENT") throw error; }); }