From d0c4968e971765004939737d9ea46f8874acdaaf Mon Sep 17 00:00:00 2001 From: Priya Raman Date: Thu, 20 Aug 2026 06:19:27 +0000 Subject: [PATCH] fix(sandbox): make a post-commit response-read failure non-retryable The bridge forward reads the host response body after the fetch resolves. A mutating request may have committed on the host by then. A body over the size limit, or a stream read error, made the forward reject. The duplex broker and the file bridge both mapped that rejection to a retryable 502. A retry then sent a new request id outside the broker deduplication set, so the host applied the mutation twice. The forward now catches a response-body read failure and returns a non-retryable 504 with the indeterminate outcome marker. The in-sandbox server maps the indeterminate 504 to a non-retryable 409 for both transports, so a caller never repeats a possibly-committed mutation. Add broker and integration coverage. Co-authored-by: Paperclip --- .../src/duplex-bridge-broker.test.ts | 52 +++++++++++++++++++ .../adapter-utils/src/duplex-bridge-broker.ts | 9 ++-- .../src/execution-target-sandbox.test.ts | 38 ++++++++++---- .../adapter-utils/src/execution-target.ts | 27 +++++++++- 4 files changed, 112 insertions(+), 14 deletions(-) diff --git a/packages/adapter-utils/src/duplex-bridge-broker.test.ts b/packages/adapter-utils/src/duplex-bridge-broker.test.ts index cf00f6ebb7a..e8bfeb78f25 100644 --- a/packages/adapter-utils/src/duplex-bridge-broker.test.ts +++ b/packages/adapter-utils/src/duplex-bridge-broker.test.ts @@ -367,4 +367,56 @@ describe("duplex bridge broker request limits", () => { expect(harness.forwards.filter((forward) => forward.id === maxId)).toHaveLength(1); expect(broker.state).toBe("open"); }); + + it("maps a forward that resolves with the indeterminate marker to a non-retryable response and retains the id", async () => { + const harness = createFakeChannelHarness(); + const broker = createDuplexBridgeBroker({ + channel: harness.channel, + forwardRequest: controllableForward(harness), + }); + brokers.push(broker); + broker.start(); + + // The forward handler resolves with a 504 that carries the indeterminate + // marker. This is the shape the host forward returns for a response-read + // failure after the host commit. The broker must keep the marker and the + // non-retryable body, so the gateway maps it to a non-retryable status and a + // caller never repeats a possibly-committed mutation. + harness.feed(requestFrame("commit-1")); + const forward = harness.forwards.find((entry) => entry.id === "commit-1" && !entry.settled); + if (!forward) throw new Error("The broker did not forward the request."); + forward.settled = true; + forward.resolve({ + status: 504, + headers: { + "content-type": "application/json", + "x-paperclip-bridge-outcome": "indeterminate", + }, + body: JSON.stringify({ + error: "Bridge response body exceeded the configured size limit of 32 bytes.", + outcome: "indeterminate", + retryable: false, + }), + }); + // The broker answers inside the forward `then` microtask, so let it settle. + await Promise.resolve(); + + expect(harness.responses).toHaveLength(1); + const response = harness.responses[0]!; + expect(response.status).toBe(504); + expect(response.outcome).toBe("indeterminate"); + expect(response.headers["x-paperclip-bridge-outcome"]).toBe("indeterminate"); + expect(JSON.parse(response.body)).toEqual({ + error: "Bridge response body exceeded the configured size limit of 32 bytes.", + outcome: "indeterminate", + retryable: false, + }); + + // The broker delivered a response, so it retained the id. A resend never + // reaches the forward a second time, so a caller that ignores the + // non-retryable status still cannot repeat the mutation through the broker. + harness.feed(requestFrame("commit-1")); + expect(harness.forwards.filter((entry) => entry.id === "commit-1")).toHaveLength(1); + expect(broker.state).toBe("open"); + }); }); diff --git a/packages/adapter-utils/src/duplex-bridge-broker.ts b/packages/adapter-utils/src/duplex-bridge-broker.ts index c51da96b9b6..5d01b1d5bab 100644 --- a/packages/adapter-utils/src/duplex-bridge-broker.ts +++ b/packages/adapter-utils/src/duplex-bridge-broker.ts @@ -535,9 +535,12 @@ export function createDuplexBridgeBroker(options: DuplexBrokerOptions): DuplexBr ); return; } - // A plain forward failure did not start a host mutation, so a retry is - // safe. Return a 502 with the completed outcome, so the gateway passes it - // through as a retryable status. + // A rejection here means the forward never received a host response, so + // the host did not start a mutation and a retry is safe. The forward + // handler owns the post-response case: it converts a response-read + // failure into a resolved indeterminate result, so a possibly-committed + // mutation never reaches this branch. Return a 502 with the completed + // outcome, so the gateway passes it through as a retryable status. respond( frame.id, { diff --git a/packages/adapter-utils/src/execution-target-sandbox.test.ts b/packages/adapter-utils/src/execution-target-sandbox.test.ts index 7c11928adb6..1a80090b747 100644 --- a/packages/adapter-utils/src/execution-target-sandbox.test.ts +++ b/packages/adapter-utils/src/execution-target-sandbox.test.ts @@ -2495,7 +2495,14 @@ describe("sandbox adapter execution targets", () => { } }); - it("fails oversized host responses with a 502 before returning them to the sandbox client", async () => { + it("fails an oversized host response with a non-retryable 409 so a committed mutation never repeats", async () => { + // The host receives the request and commits the mutation, then sends a + // response body over the size limit. The forward reads the body after the + // fetch resolves, so the read failure happens after the host commit. The + // forward must return a non-retryable 504 with the indeterminate outcome, not + // a retryable 502. The in-sandbox server maps the indeterminate 504 to a + // non-retryable 409. A retryable status would repeat the mutation with a new + // request id outside the broker deduplication set. const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-execution-target-bridge-limit-")); cleanupDirs.push(rootDir); const remoteCwd = path.join(rootDir, "workspace"); @@ -2503,7 +2510,10 @@ describe("sandbox adapter execution targets", () => { await mkdir(runtimeRootDir, { recursive: true }); const requests: Array<{ method: string; url: string; auth: string | null; runId: string | null }> = []; - const largeBody = "x".repeat(64); + // The host body sits over the size limit, so the forward read fails. The + // limit stays above the small indeterminate marker the forward returns, so the + // marker still reaches the server for the 504-to-409 map. + const largeBody = "x".repeat(1024); const apiServer = createServer((req, res) => { requests.push({ method: req.method ?? "GET", @@ -2511,7 +2521,7 @@ describe("sandbox adapter execution targets", () => { auth: req.headers.authorization ?? null, runId: typeof req.headers["x-paperclip-run-id"] === "string" ? req.headers["x-paperclip-run-id"] : null, }); - res.writeHead(200, { + res.writeHead(201, { "content-type": "application/json", "content-length": String(Buffer.byteLength(largeBody, "utf8")), }); @@ -2544,23 +2554,31 @@ describe("sandbox adapter execution targets", () => { adapterKey: "codex", hostApiToken: "real-run-jwt", hostApiUrl: `http://127.0.0.1:${address.port}`, - maxBodyBytes: 32, + maxBodyBytes: 512, }); try { - const response = await fetch(`${bridge!.env.PAPERCLIP_API_URL}/api/agents/me`, { + const response = await fetch(`${bridge!.env.PAPERCLIP_API_URL}/api/issues/issue-1/comments`, { + method: "POST", headers: { authorization: `Bearer ${bridge!.env.PAPERCLIP_API_KEY}`, - accept: "application/json", + "content-type": "application/json", }, + body: JSON.stringify({ body: "Status update." }), }); - expect(response.status).toBe(502); + // The indeterminate 504 maps to a non-retryable 409, so the caller does not + // retry the committed mutation. + expect(response.status).toBe(409); + expect(response.headers.get("x-paperclip-bridge-outcome")).toBe("indeterminate"); await expect(response.json()).resolves.toEqual({ - error: "Bridge response body exceeded the configured size limit of 32 bytes.", + error: "Bridge response body exceeded the configured size limit of 512 bytes.", + outcome: "indeterminate", + retryable: false, }); + // The host ran the mutation exactly once. It never receives a retry. expect(requests).toEqual([{ - method: "GET", - url: "/api/agents/me", + method: "POST", + url: "/api/issues/issue-1/comments", auth: "Bearer real-run-jwt", runId: "run-bridge-limit", }]); diff --git a/packages/adapter-utils/src/execution-target.ts b/packages/adapter-utils/src/execution-target.ts index ce16c24e26d..380bf5a8406 100644 --- a/packages/adapter-utils/src/execution-target.ts +++ b/packages/adapter-utils/src/execution-target.ts @@ -2631,7 +2631,32 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: { `[paperclip] Bridge proxy response ${response.status} for ${method} ${request.path}${request.query ? `?${request.query}` : ""}\n`, ); } - const responseBody = await readBridgeForwardResponseBody(response, maxBodyBytes); + // The host delivered response headers, so a mutating request may have + // committed on the host. A later response-body read failure (a body over the + // size limit, or a stream read error) must not surface as a retryable status. + // A retryable status makes the caller repeat the request with a new request + // id outside the broker deduplication set, so the host applies the mutation + // twice. Return a non-retryable 504 and mark the outcome indeterminate, + // exactly like an aborted in-flight forward. The in-sandbox server maps the + // indeterminate 504 to a non-retryable 409 for both the file bridge and the + // duplex broker. + let responseBody: string; + try { + responseBody = await readBridgeForwardResponseBody(response, maxBodyBytes); + } catch (error) { + return { + status: 504, + headers: { + "content-type": "application/json", + "x-paperclip-bridge-outcome": "indeterminate", + }, + body: JSON.stringify({ + error: error instanceof Error ? error.message : String(error), + outcome: "indeterminate", + retryable: false, + }), + }; + } const commentMarker = postedIssueCommentLogMarker(method, request.path, response.status, responseBody); if (commentMarker) await onLog("stdout", commentMarker); return {