From dc7a1a020a799ef47f25d184a8748a760fd8cedd Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Thu, 27 Aug 2026 22:08:08 -0700 Subject: [PATCH] fix(adapter-utils): skip the remote session close when the duplex channel is already lost (#12394) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The adapter runtime settles each run through a duplex control channel > - A lost channel can leave the remote session-close call without a usable peer > - The call has no deadline, so run teardown can wait for the full adapter timeout > - This pull request skips that remote call after the runtime latches channel loss > - The benefit is faster run finalization while the local cleanup effects remain ## Linked Issues or Issue Description **What happened?** The run teardown placed a remote session-close call over a duplex control channel that the runtime had already latched as lost. The call blocked until the adapter execution timeout released it. **Expected behavior** Run teardown should release the local warm handle and continue when the duplex control channel has already failed. **Steps to reproduce** 1. Start an adapter run with the duplex control channel. 2. Latch a channel-loss state before settlement. 3. Use a runtime whose close call never resolves. 4. Confirm that teardown returns without a remote close call. **Paperclip version or commit** Commit d966069a788e714b76adcc39aab0e0b26909d89c. **Deployment mode** Built from source. **Agent adapter(s) involved** Not adapter-specific. The change applies to the shared adapter runtime. **Database mode** Not database-related. **Additional context** Pull request #12373 used a larger approach for the same failure. The board closed that pull request. This pull request contains the smaller change. ## What Changed - Add the required readonly `skipRemoteClose` field to the runtime settlement plan. - Set the field from the latched channel-loss state on the turn-finalize plan. - Set the field to `false` on every other settlement plan. - Release the warm handle locally before the `end_session` step returns without the remote call. - Add a test that drives the lost-channel path through the settlement sequence. ## Verification - `./node_modules/.bin/tsc --noEmit -p packages/adapter-utils` - `./node_modules/.bin/vitest run packages/adapter-utils/src/acpx-engine/execute.test.ts` passes the new test. Three existing tests fail on the merge base: two session-fingerprint tests and one workspace-hints test. - `./node_modules/.bin/vitest run packages/adapter-utils/src/acpx-engine/run-fault-matrix.test.ts` reports 20 passed. - Full CI will run on this pull request. ## Risks The skipped remote close also skips the vendored runtime caller for `closeBackendSession`. The run can keep a retained client after duplex loss. A separate follow-up owns that residual. The environment lease still releases in the teardown `finally` block. ## Model Used OpenAI Codex, GPT-5, with tool use and code review assistance. ## 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/acpx-engine/execute.test.ts | 60 +++++++++++++++++++ .../adapter-utils/src/acpx-engine/execute.ts | 31 ++++++++++ 2 files changed, 91 insertions(+) diff --git a/packages/adapter-utils/src/acpx-engine/execute.test.ts b/packages/adapter-utils/src/acpx-engine/execute.test.ts index 5575b37b46..c47d057e3e 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.test.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.test.ts @@ -6108,4 +6108,64 @@ describe("ACPX engine sandbox bridge run-disposition seam (fail-closed)", () => fake.emitLoss("provider_exit"); expect(fake.readDisposition().failed).toBe(false); }); + + it("releases the runtime locally and places no remote close call once the duplex channel is lost", async () => { + const sandbox = await setupRemoteSandbox(); + const fake = createFakeBridgeHandle(); + let closeCalls = 0; + const runtime = runtimeWithControlledResult(() => fake.emitLoss("provider_exit")); + // A close call with no deadline of its own would hang forever on a dead + // channel. The test times out if the teardown still places the call. + runtime.close = () => { + closeCalls += 1; + return new Promise(() => {}); + }; + + const result = await runRemote(fake.handle, runtime, sandbox); + + expect(result.errorCode).toBe("duplex_channel_lost"); + expect(closeCalls).toBe(0); + }); + + it("skips the remote close when the channel loses during the awaited turn-error finalization, after the settlement snapshot", async () => { + const sandbox = await setupRemoteSandbox(); + const fake = createFakeBridgeHandle(); + let closeCalls = 0; + // The event stream errors out (a `turnFinalize` "error" input, not a + // "terminal" one), so the settlement snapshot never inspects the bridge + // disposition and always sets `skipRemoteClose: false`. Order the channel + // loss inside the event stream, right before it throws, so the loss + // latches strictly after that snapshot and before the end-session step + // reaches its remote-close boundary. + const runtime = { + ensureSession: async () => ({ + backendSessionId: "backend-session", + agentSessionId: "agent-session", + runtimeSessionName: "runtime-session", + }), + startTurn: () => ({ + events: (async function* () { + fake.emitLoss("provider_exit"); + throw new Error("turn upstream boom"); + })(), + result: Promise.resolve({ status: "completed" as const, stopReason: "end_turn" }), + cancel: async () => {}, + }), + setConfigOption: async () => {}, + // A close call with no deadline of its own would hang forever on a dead + // channel. The test fails on a nonzero call count instead of timing out. + close: () => { + closeCalls += 1; + return new Promise(() => {}); + }, + }; + + const result = await runRemote(fake.handle, runtime, sandbox); + + expect(result.errorCode).toBe("acpx_turn_failed"); + // The end-session step re-read the disposition at the remote-close + // boundary and saw the loss the settlement snapshot missed, so it + // released the runtime locally and placed no remote close call. + expect(closeCalls).toBe(0); + }); }); diff --git a/packages/adapter-utils/src/acpx-engine/execute.ts b/packages/adapter-utils/src/acpx-engine/execute.ts index 29261ea596..65a77de060 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.ts @@ -2405,6 +2405,10 @@ interface RuntimeSettlementPlan { // Cancel the running turn with this reason before the close (the turn-error // path cancels before it closes). Null on every other path. readonly cancelTurnReason: string | null; + // True when the duplex control channel is already known lost. The settlement + // then releases the runtime locally and places no remote close call, because + // that call has no deadline of its own and would block on the dead channel. + readonly skipRemoteClose: boolean; } function renderPaperclipEnvNote(env: Record): string { @@ -3803,6 +3807,7 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { dropWarmEntry: true, recordCloseError: true, cancelTurnReason: null, + skipRemoteClose: false, }; await emitPhase("ensure_session", ensureSessionPhaseStart, "failed"); const { classified, message } = await emitAcpxFailure({ @@ -3839,6 +3844,7 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { dropWarmEntry: false, recordCloseError: true, cancelTurnReason: null, + skipRemoteClose: false, }; await emitPhase("ensure_session", ensureSessionPhaseStart, "failed"); capturedResult = { @@ -3922,6 +3928,7 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { dropWarmEntry: true, recordCloseError: true, cancelTurnReason: null, + skipRemoteClose: false, }; await emitPhase("configure_session", configureSessionStart, "failed"); const { classified, message } = await emitAcpxFailure({ @@ -4175,6 +4182,7 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { dropWarmEntry: false, recordCloseError: false, cancelTurnReason: null, + skipRemoteClose: channelLost, }; const errorMessage = timedOut @@ -4313,6 +4321,7 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { dropWarmEntry: true, recordCloseError: true, cancelTurnReason: preEmitMessage, + skipRemoteClose: false, }; // Emit the failure best-effort. `turnFinalize` must not reject, so a // failing emission never propagates: the settlement owns the teardown, and @@ -4420,12 +4429,34 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { dropWarmEntry: false, recordCloseError: true, cancelTurnReason: null, + skipRemoteClose: false, }; // Cancel a running turn before the close (the turn-error path). if (settlement.cancelTurnReason && activeTurn) { await activeTurn.cancel({ reason: settlement.cancelTurnReason }).catch(() => {}); } const existing = warmHandles.get(prepared.sessionKey); + // Re-read the duplex control-channel disposition here, at the + // boundary that places the remote call. The settlement snapshot + // above can predate a channel loss the bridge latches during the + // awaited finalization work between the snapshot and this point, so + // a stale `false` on the snapshot must not force a call onto a + // channel that is dead by now. The read is non-mutating and only + // adds a later-observed loss; it never clears the snapshot's `true`. + const remoteChannelLost = + settlement.skipRemoteClose || (prepared.paperclipBridge?.readRunDisposition?.().failed ?? false); + // The control channel is already known lost, so no remote call can + // reach the backend. Release the local bookkeeping only and place no + // `runtime.close(...)` call — that call has no deadline of its own + // and would block on the dead channel. + if (remoteChannelLost) { + if (warmHandleMatches(existing, runtime, settlement.handle) && existing) { + clearWarmHandleTimer(existing); + warmHandles.delete(prepared.sessionKey); + flushChildStderr(existing.childStderrState); + } + return; + } if ( settlement.mode === "warm_or_close" && warmHandleMatches(existing, runtime, settlement.handle) &&