From f1d9206c4ac32771d7663fb9a4d8865a200f3dd5 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Thu, 3 Sep 2026 03:20:54 -0500 Subject: [PATCH] fix(runner): retain aborted admission cleanup (#12755) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip manages AI agents and their work. > - The runner starts ACPX sessions and controls their resources. > - An aborted admission can leave sandbox preparation active after the opening promise rejects. > - Test teardown can then remove the sandbox directory before that work ends. > - This pull request retains and observes each unfinished admission stage. > - The change gives runtime resources and temporary directories one deterministic cleanup owner. ## Linked Issues or Issue Description **What happened?** Under full test load, an aborted admission test can end before sandbox preparation settles. Test teardown then removes the temporary session directory. The active preparation can report an unhandled `ENOENT` error. **Expected behavior** An aborted admission must observe and retain all active preparation work. Test teardown must wait until that work settles. **Steps to reproduce** 1. Run the complete `@paperclipai/paperclip-runner` test suite under CI load. 2. Abort runtime admission during credential or sandbox preparation. 3. Observe an intermittent test timeout or an unhandled missing-directory error. **Paperclip version or commit** The failure occurred on a branch based on commit `b1f4910ee`. This fix is based on current `master` commit `4d30efa8e`. **Deployment mode** The failure occurred in GitHub Actions on a source build. ## What Changed - Retain each unfinished abortable admission stage in the global runtime-host cleanup set. - Notify the embedding lifecycle when an aborted stage needs deferred cleanup. - Make test teardown abort and await all active opening and cleanup promises before directory removal. - Replace time-based stage detection with exact deferred stage signals. - Add a deterministic regression test for an abort during sandbox preparation. ## Verification - Ran the focused runtime-host file in 20 separate processes. All 20 runs passed without an unhandled error. - Ran `pnpm --filter @paperclipai/paperclip-runner exec vitest run src/drivers/acpx/runtime-host.test.ts` after the rebase. All 27 tests passed. - Ran `pnpm --filter @paperclipai/paperclip-runner check:all` after the rebase. The full command passed. - The final TypeScript test stage passed 127 files and 1,490 tests. All Rust checks, tests, and parity checks passed. - Greptile reviewed two heads. The final review is 5/5 with no open comments. - All latest-head CI and security checks passed. One unrelated workspace test passed on its permitted rerun. ## Risks - Risk is low. An aborted stage now delays final runtime-host cleanup until its active operation settles. - A stage that never settles can delay embedding shutdown. The existing stage operations have bounded or controlled owners. - The regression test holds sandbox preparation and confirms the new cleanup order. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used OpenAI Codex with GPT-5 assisted this change. The environment did not provide the exact deployment ID or context size. The model used reasoning, shell tools, code editing, and test execution. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- .../src/drivers/acpx/runtime-host.test.ts | 322 +++++++++++++----- .../src/drivers/acpx/runtime-host.ts | 72 ++-- 2 files changed, 296 insertions(+), 98 deletions(-) diff --git a/packages/paperclip-runner/src/drivers/acpx/runtime-host.test.ts b/packages/paperclip-runner/src/drivers/acpx/runtime-host.test.ts index f5eb29f0bf..8ec2e7cbdc 100644 --- a/packages/paperclip-runner/src/drivers/acpx/runtime-host.test.ts +++ b/packages/paperclip-runner/src/drivers/acpx/runtime-host.test.ts @@ -11,6 +11,7 @@ import type { VerifiedAcpxInstallation, } from "./installation-integrity.js"; import { resolveQualifiedAcpxProfile } from "./qualified-profiles.js"; +import { prepareAcpxRuntimeSandbox } from "./runtime-sandbox.js"; import { AcpxRuntimeHost, type AcpxRuntimeHostDependencies, @@ -20,8 +21,18 @@ import { } from "./runtime-host.js"; const temporaryDirectories: string[] = []; +const admissionControllers: AbortController[] = []; +const pendingAdmissionOpenings = new Set>(); +const pendingAdmissionCleanups = new Set>(); afterEach(async () => { + for (const controller of admissionControllers.splice(0)) { + if (!controller.signal.aborted) { + controller.abort(new Error("ACPX runtime host test cleanup")); + } + } + await Promise.all([...pendingAdmissionOpenings]); + await Promise.all([...pendingAdmissionCleanups]); await Promise.all( temporaryDirectories .splice(0) @@ -32,7 +43,7 @@ afterEach(async () => { describe("ACPX runtime host", () => { it("rejects a pre-aborted admission before acquiring provider resources", async () => { const fixture = await hostFixture(); - const controller = new AbortController(); + const controller = trackedAdmissionController(); const cancellation = new Error("admission cancelled before start"); controller.abort(cancellation); const openRuntime = vi.fn(async () => runtimePort()); @@ -61,9 +72,115 @@ describe("ACPX runtime host", () => { expect(fixture.commandClose).not.toHaveBeenCalled(); }); + it("retains aborted sandbox preparation until its filesystem work settles", async () => { + const fixture = await hostFixture(); + const controller = trackedAdmissionController(); + const cancellation = new Error("sandbox admission cancelled"); + const sandboxStarted = deferred(); + const finishSandbox = deferred(); + const retainedCleanups: Promise[] = []; + const stageCredential = vi.fn(async () => { + throw new Error("credential staging must not start"); + }); + const openRuntime = vi.fn(async () => runtimePort()); + const dependencies = fixture.dependencies({ openRuntime }); + dependencies.stageCredential = stageCredential; + dependencies.prepareSandbox = async (input) => { + sandboxStarted.resolve(undefined); + await finishSandbox.promise; + return await prepareAcpxRuntimeSandbox(input); + }; + dependencies.retainAdmissionCleanup = (cleanup) => { + retainedCleanups.push(cleanup); + trackAdmissionCleanup(cleanup); + }; + const opening = trackAdmissionOpening( + AcpxRuntimeHost.open( + { + ...fixture.options, + agent: "codex", + model: "gpt-5.6-sol", + permissionMode: "deny-all", + signal: controller.signal, + }, + dependencies, + ), + ); + await sandboxStarted.promise; + + controller.abort(cancellation); + await expect(opening).rejects.toBe(cancellation); + expect(retainedCleanups).toHaveLength(2); + let sandboxCleanupSettled = false; + void retainedCleanups[0]!.then( + () => { + sandboxCleanupSettled = true; + }, + () => { + sandboxCleanupSettled = true; + }, + ); + await Promise.resolve(); + expect(sandboxCleanupSettled).toBe(false); + + finishSandbox.resolve(undefined); + await expect(retainedCleanups[0]).resolves.toBeUndefined(); + expect(sandboxCleanupSettled).toBe(true); + expect(stageCredential).not.toHaveBeenCalled(); + expect(openRuntime).not.toHaveBeenCalled(); + expect(fixture.commandClose).not.toHaveBeenCalled(); + }); + + it("settles retained admission work when aborted sandbox preparation rejects", async () => { + const fixture = await hostFixture(); + const controller = trackedAdmissionController(); + const cancellation = new Error("sandbox admission cancelled"); + const sandboxStarted = deferred(); + const finishSandbox = deferred(); + const retainedCleanups: Promise[] = []; + const stageCredential = vi.fn(async () => { + throw new Error("credential staging must not start"); + }); + const openRuntime = vi.fn(async () => runtimePort()); + const dependencies = fixture.dependencies({ openRuntime }); + dependencies.stageCredential = stageCredential; + dependencies.prepareSandbox = async (input) => { + sandboxStarted.resolve(undefined); + await finishSandbox.promise; + return await prepareAcpxRuntimeSandbox(input); + }; + dependencies.retainAdmissionCleanup = (cleanup) => { + retainedCleanups.push(cleanup); + trackAdmissionCleanup(cleanup); + }; + const opening = trackAdmissionOpening( + AcpxRuntimeHost.open( + { + ...fixture.options, + agent: "codex", + model: "gpt-5.6-sol", + permissionMode: "deny-all", + signal: controller.signal, + }, + dependencies, + ), + ); + await sandboxStarted.promise; + + controller.abort(cancellation); + await expect(opening).rejects.toBe(cancellation); + expect(retainedCleanups).toHaveLength(2); + + finishSandbox.reject(new Error("sandbox preparation failed after abort")); + await expect(retainedCleanups[0]).resolves.toBeUndefined(); + expect(stageCredential).not.toHaveBeenCalled(); + expect(openRuntime).not.toHaveBeenCalled(); + expect(fixture.commandClose).not.toHaveBeenCalled(); + }); + it("scrubs credentials when abort wins before the adapter body starts", async () => { const fixture = await hostFixture(); - const controller = new AbortController(); + const controller = trackedAdmissionController(); const cancellation = new Error("runtime admission cancelled before entry"); const createRuntime = vi.fn(); let credentialHome = ""; @@ -919,32 +1036,39 @@ describe("ACPX runtime host", () => { it("closes a command lease that resolves after admission is aborted", async () => { const fixture = await hostFixture(); const commandAdmission = deferred(); + const commandAdmissionStarted = deferred(); const lateCommandClose = vi.fn(async () => undefined); - const openCommand = vi.fn(() => commandAdmission.promise); + const openCommand = vi.fn(() => { + commandAdmissionStarted.resolve(undefined); + return commandAdmission.promise; + }); const openRuntime = vi.fn(async () => runtimePort()); - const controller = new AbortController(); + const controller = trackedAdmissionController(); const cancellation = new Error("command admission cancelled"); const profile = resolveQualifiedAcpxProfile("claude", "claude-sonnet-5"); - const opening = AcpxRuntimeHost.open( - { - ...fixture.options, - agent: "claude", - model: "claude-sonnet-5", - permissionMode: "deny-all", - signal: controller.signal, - }, - { - verifyInstallation: async () => ({ - commandDigest: profile.commandDigest, - agentServerPackageJsonPath: join(fixture.root, "package.json"), - agentRuntimePackageJsonPath: null, - openCommand, - }), - openRuntime, - reportRetainedCleanupFailure: vi.fn(), - }, + const opening = trackAdmissionOpening( + AcpxRuntimeHost.open( + { + ...fixture.options, + agent: "claude", + model: "claude-sonnet-5", + permissionMode: "deny-all", + signal: controller.signal, + }, + { + verifyInstallation: async () => ({ + commandDigest: profile.commandDigest, + agentServerPackageJsonPath: join(fixture.root, "package.json"), + agentRuntimePackageJsonPath: null, + openCommand, + }), + openRuntime, + retainAdmissionCleanup: trackAdmissionCleanup, + reportRetainedCleanupFailure: vi.fn(), + }, + ), ); - await vi.waitFor(() => expect(openCommand).toHaveBeenCalledOnce()); + await commandAdmissionStarted.promise; controller.abort(cancellation); await expect(opening).rejects.toBe(cancellation); @@ -978,27 +1102,33 @@ describe("ACPX runtime host", () => { await rm(lateCredentialPath); }); const reportRetainedCleanupFailure = vi.fn(); - const stageCredential = vi.fn(() => credentialAdmission.promise); + const credentialAdmissionStarted = deferred(); + const stageCredential = vi.fn(() => { + credentialAdmissionStarted.resolve(undefined); + return credentialAdmission.promise; + }); const openRuntime = vi.fn(async () => runtimePort()); - const controller = new AbortController(); + const controller = trackedAdmissionController(); const cancellation = new Error("credential admission cancelled"); - const opening = AcpxRuntimeHost.open( - { - ...fixture.options, - agent: "codex", - model: "gpt-5.6-sol", - permissionMode: "deny-all", - signal: controller.signal, - }, - { - ...fixture.dependencies({ - openRuntime, - reportRetainedCleanupFailure, - }), - stageCredential, - }, + const opening = trackAdmissionOpening( + AcpxRuntimeHost.open( + { + ...fixture.options, + agent: "codex", + model: "gpt-5.6-sol", + permissionMode: "deny-all", + signal: controller.signal, + }, + { + ...fixture.dependencies({ + openRuntime, + reportRetainedCleanupFailure, + }), + stageCredential, + }, + ), ); - await vi.waitFor(() => expect(stageCredential).toHaveBeenCalledOnce()); + await credentialAdmissionStarted.promise; controller.abort(cancellation); await expect(opening).rejects.toBe(cancellation); @@ -1031,6 +1161,7 @@ describe("ACPX runtime host", () => { it("retains managed credentials until an aborted late runtime is closed", async () => { const fixture = await hostFixture(); const runtimeAdmission = deferred(); + const runtimeAdmissionStarted = deferred(); const retryClose = deferred(); const lateRuntime = runtimePort({ onClose: vi @@ -1045,28 +1176,31 @@ describe("ACPX runtime host", () => { receivedSignal = options.signal; credentialHome = options.launchEnvironment.CODEX_HOME!; bridgeUrl = options.mcpServers[0]!.url; + runtimeAdmissionStarted.resolve(undefined); return runtimeAdmission.promise; }); - const controller = new AbortController(); + const controller = trackedAdmissionController(); const cancellation = new Error("runtime admission cancelled"); - const opening = AcpxRuntimeHost.open( - { - ...fixture.options, - agent: "codex", - model: "gpt-5.6-sol", - permissionMode: "deny-all", - environment: { - PAPERCLIP_ACPX_CODEX_AUTH_JSON_SECRET: "{}", + const opening = trackAdmissionOpening( + AcpxRuntimeHost.open( + { + ...fixture.options, + agent: "codex", + model: "gpt-5.6-sol", + permissionMode: "deny-all", + environment: { + PAPERCLIP_ACPX_CODEX_AUTH_JSON_SECRET: "{}", + }, + signal: controller.signal, + semanticTools: { + tools: [], + handler: async () => ({ ok: true }), + }, }, - signal: controller.signal, - semanticTools: { - tools: [], - handler: async () => ({ ok: true }), - }, - }, - fixture.dependencies({ openRuntime }), + fixture.dependencies({ openRuntime }), + ), ); - await vi.waitFor(() => expect(openRuntime).toHaveBeenCalledOnce()); + await runtimeAdmissionStarted.promise; expect(receivedSignal).toBe(controller.signal); controller.abort(cancellation); @@ -1121,34 +1255,38 @@ describe("ACPX runtime host", () => { it("scrubs credentials after rejected runtime cleanup is proven", async () => { const fixture = await hostFixture(); const runtimeAdmission = deferred(); + const runtimeAdmissionStarted = deferred(); const providerCleanup = deferred(); const credentialClose = vi.fn(async () => undefined); - const controller = new AbortController(); + const controller = trackedAdmissionController(); const cancellation = new Error("runtime admission cancelled"); const openRuntime = vi.fn((options: AcpxRuntimePortOpenOptions) => { options.retainFailedAdmissionCleanup(providerCleanup.promise); + runtimeAdmissionStarted.resolve(undefined); return runtimeAdmission.promise; }); - const opening = AcpxRuntimeHost.open( - { - ...fixture.options, - agent: "codex", - model: "gpt-5.6-sol", - permissionMode: "deny-all", - signal: controller.signal, - }, - { - ...fixture.dependencies({ openRuntime }), - stageCredential: async () => ({ - path: join(fixture.root, "auth.json"), - mode: "inline_json", - lifetimeFenceFds: [42, 43], - activateLifetimeOwner: async () => undefined, - close: credentialClose, - }), - }, + const opening = trackAdmissionOpening( + AcpxRuntimeHost.open( + { + ...fixture.options, + agent: "codex", + model: "gpt-5.6-sol", + permissionMode: "deny-all", + signal: controller.signal, + }, + { + ...fixture.dependencies({ openRuntime }), + stageCredential: async () => ({ + path: join(fixture.root, "auth.json"), + mode: "inline_json", + lifetimeFenceFds: [42, 43], + activateLifetimeOwner: async () => undefined, + close: credentialClose, + }), + }, + ), ); - await vi.waitFor(() => expect(openRuntime).toHaveBeenCalledOnce()); + await runtimeAdmissionStarted.promise; controller.abort(cancellation); await expect(opening).rejects.toBe(cancellation); @@ -1245,6 +1383,7 @@ async function hostFixture() { openCommand: async () => command, }) satisfies VerifiedAcpxInstallation, openRuntime: input.openRuntime, + retainAdmissionCleanup: trackAdmissionCleanup, reportRetainedCleanupFailure: input.reportRetainedCleanupFailure ?? vi.fn(), }; @@ -1252,6 +1391,33 @@ async function hostFixture() { }; } +function trackedAdmissionController(): AbortController { + const controller = new AbortController(); + admissionControllers.push(controller); + return controller; +} + +function trackAdmissionOpening(opening: Promise): Promise { + trackSettledPromise(pendingAdmissionOpenings, opening); + return opening; +} + +function trackAdmissionCleanup(cleanup: Promise): void { + trackSettledPromise(pendingAdmissionCleanups, cleanup); +} + +function trackSettledPromise( + pending: Set>, + promise: Promise, +): void { + const observed = promise.then( + () => undefined, + () => undefined, + ); + pending.add(observed); + void observed.finally(() => pending.delete(observed)); +} + function deferred(): { promise: Promise; resolve(value: T): void; diff --git a/packages/paperclip-runner/src/drivers/acpx/runtime-host.ts b/packages/paperclip-runner/src/drivers/acpx/runtime-host.ts index c568b613b1..3c9b54c137 100644 --- a/packages/paperclip-runner/src/drivers/acpx/runtime-host.ts +++ b/packages/paperclip-runner/src/drivers/acpx/runtime-host.ts @@ -142,6 +142,8 @@ export interface AcpxRuntimeHostDependencies { /** Internal test seam for aborting credential acquisition. */ stageCredential?: typeof stageManagedCodexCredential; openRuntime(options: AcpxRuntimePortOpenOptions): Promise; + /** Internal test seam for deterministic sandbox-admission scheduling. */ + prepareSandbox?: typeof prepareAcpxRuntimeSandbox; /** Internal test seam for the post-handshake admission deadline. */ admissionVerificationTimeoutMs?: number; /** Internal test seam for failed-admission cleanup. */ @@ -251,15 +253,18 @@ export class AcpxRuntimeHost { ); } const profile = resolveQualifiedAcpxProfile(options.agent, options.model); - const binding = await runAbortableAdmissionStage(options.signal, () => - createAcpxRecoveryBinding({ - runtimeDirectory: options.runtimeDirectory, - normalizedSessionId: options.normalizedSessionId, - workingDirectory: options.workingDirectory, - profile, - requestedModel: options.model, - permissionMode: options.permissionMode, - }), + const binding = await runAbortableAdmissionStage( + options.signal, + () => + createAcpxRecoveryBinding({ + runtimeDirectory: options.runtimeDirectory, + normalizedSessionId: options.normalizedSessionId, + workingDirectory: options.workingDirectory, + profile, + requestedModel: options.model, + permissionMode: options.permissionMode, + }), + dependencies.retainAdmissionCleanup, ); if (options.expectedIdentity) { verifyExpectedAcpxIdentity(options.expectedIdentity, binding, null); @@ -274,10 +279,13 @@ export class AcpxRuntimeHost { ); } - const installation = await runAbortableAdmissionStage(options.signal, () => - (dependencies.verifyInstallation ?? verifyQualifiedAcpxInstallation)( - profile, - ), + const installation = await runAbortableAdmissionStage( + options.signal, + () => + (dependencies.verifyInstallation ?? verifyQualifiedAcpxInstallation)( + profile, + ), + dependencies.retainAdmissionCleanup, ); if (installation.commandDigest !== profile.commandDigest) { throw new Error("Verified ACPX installation does not match its profile"); @@ -324,12 +332,15 @@ export class AcpxRuntimeHost { retainRuntimeHostCleanup(ownedCleanup); }; try { - const sandbox = await runAbortableAdmissionStage(options.signal, () => - prepareAcpxRuntimeSandbox({ - binding, - agent: options.agent, - environment: options.environment, - }), + const sandbox = await runAbortableAdmissionStage( + options.signal, + () => + (dependencies.prepareSandbox ?? prepareAcpxRuntimeSandbox)({ + binding, + agent: options.agent, + environment: options.environment, + }), + dependencies.retainAdmissionCleanup, ); if (options.agent === "codex") { credential = await acquireAbortableAdmissionResource({ @@ -445,6 +456,7 @@ export class AcpxRuntimeHost { profile, admissionVerificationTimeoutMs, ), + dependencies.retainAdmissionCleanup, ); const observedIdentity: AcpxExpectedSessionIdentity = { kind: "acpx", @@ -631,11 +643,31 @@ export class AcpxRuntimeHost { async function runAbortableAdmissionStage( signal: AbortSignal | undefined, operation: () => Promise, + retainCleanup: ((cleanup: Promise) => void) | undefined, ): Promise { if (signal === undefined) return await operation(); signal.throwIfAborted(); const pending = Promise.resolve().then(operation); - return await raceAdmissionWithAbort(pending, signal); + try { + return await raceAdmissionWithAbort(pending, signal); + } catch (error) { + if (signal.aborted) { + // Abort may win while sandbox preparation or another non-resource stage + // still owns asynchronous work. Keep that exact operation observed and + // expose it to the embedding lifecycle so filesystem teardown cannot + // remove its session root while it is still making durable writes. + // The aborted opening is already authoritative, and this stage owns no + // provider resource. Its retained promise represents settlement only; + // a late stage rejection must not masquerade as failed resource cleanup. + const cleanup = pending.then( + () => undefined, + () => undefined, + ); + retainRuntimeHostCleanup(cleanup); + retainCleanup?.(cleanup); + } + throw error; + } } async function acquireAbortableAdmissionResource(input: {