From f09d5ba416fa84342bd18174eec700b5dec58d65 Mon Sep 17 00:00:00 2001 From: Priya Raman Date: Thu, 20 Aug 2026 06:28:17 +0000 Subject: [PATCH] fix(sandbox): make a pre-response duplex forward failure non-retryable for a mutating method The duplex broker forwarded each request and mapped a forward rejection to a retryable 502 when its controller was not aborted. A fetch can reject after the request bytes reach the host and the host commits a mutation, but before the response headers arrive. A retry then sent a new request id outside the broker deduplication set, so the host applied the mutation twice. The broker now classifies a non-aborted forward rejection by method safety. A safe method (GET, HEAD, OPTIONS, TRACE) never changes host state, so its rejection stays a retryable 502. Any other method may have committed, so the broker returns a non-retryable 504 and marks the outcome indeterminate. The in-sandbox gateway maps the indeterminate outcome to a terminal 409, so a caller never repeats a possibly-committed mutation. Add broker coverage for both paths. Co-authored-by: Paperclip --- .../src/duplex-bridge-broker.test.ts | 74 ++++++++++++++++++- .../adapter-utils/src/duplex-bridge-broker.ts | 58 ++++++++++++--- 2 files changed, 120 insertions(+), 12 deletions(-) diff --git a/packages/adapter-utils/src/duplex-bridge-broker.test.ts b/packages/adapter-utils/src/duplex-bridge-broker.test.ts index e8bfeb78f25..f3c95d19b87 100644 --- a/packages/adapter-utils/src/duplex-bridge-broker.test.ts +++ b/packages/adapter-utils/src/duplex-bridge-broker.test.ts @@ -57,12 +57,12 @@ interface FakeChannelHarness { } /** Build one valid request frame with distinctive, secret-looking fields. */ -function requestFrame(id: string): DuplexRequestFrame { +function requestFrame(id: string, method = "POST"): DuplexRequestFrame { return { version: DUPLEX_FRAME_VERSION, type: "request", id, - method: "POST", + method, path: `/api/issues/${id}`, query: "?secret-query=leak", headers: { authorization: "Bearer super-secret-provider-token" }, @@ -419,4 +419,74 @@ describe("duplex bridge broker request limits", () => { expect(harness.forwards.filter((entry) => entry.id === "commit-1")).toHaveLength(1); expect(broker.state).toBe("open"); }); + + it("maps a forward rejection for a mutating method to a non-retryable indeterminate 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 rejects before the host delivers a response. A fetch can + // reject after the request reaches the host and the host commits, but + // before the response headers arrive. The broker did not abort the + // forward, so a POST must return a non-retryable indeterminate response. + // A retryable status would let a caller repeat a committed mutation. + harness.feed(requestFrame("mutate-1", "POST")); + const forward = harness.forwards.find((entry) => entry.id === "mutate-1" && !entry.settled); + if (!forward) throw new Error("The broker did not forward the request."); + forward.settled = true; + forward.reject(new Error("fetch failed before the response headers arrived")); + // The broker answers inside the forward rejection microtask, so let it settle. + await new Promise((resolve) => setTimeout(resolve, 0)); + + 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: "fetch failed before the response headers arrived", + 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("mutate-1", "POST")); + expect(harness.forwards.filter((entry) => entry.id === "mutate-1")).toHaveLength(1); + expect(broker.state).toBe("open"); + }); + + it("maps a forward rejection for a safe method to a retryable response", async () => { + const harness = createFakeChannelHarness(); + const broker = createDuplexBridgeBroker({ + channel: harness.channel, + forwardRequest: controllableForward(harness), + }); + brokers.push(broker); + broker.start(); + + // A safe method never changes host state, so a retry cannot double-apply a + // mutation. A forward rejection for a GET stays retryable: the broker + // returns a 502 with the completed outcome, so the gateway passes it through. + harness.feed(requestFrame("read-1", "GET")); + const forward = harness.forwards.find((entry) => entry.id === "read-1" && !entry.settled); + if (!forward) throw new Error("The broker did not forward the request."); + forward.settled = true; + forward.reject(new Error("fetch failed before the response headers arrived")); + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(harness.responses).toHaveLength(1); + const response = harness.responses[0]!; + expect(response.status).toBe(502); + expect(response.outcome).toBe("completed"); + expect(response.headers["x-paperclip-bridge-outcome"]).toBeUndefined(); + expect(JSON.parse(response.body)).toEqual({ + error: "fetch failed before the response headers arrived", + }); + }); }); diff --git a/packages/adapter-utils/src/duplex-bridge-broker.ts b/packages/adapter-utils/src/duplex-bridge-broker.ts index 5d01b1d5bab..b3687b2904c 100644 --- a/packages/adapter-utils/src/duplex-bridge-broker.ts +++ b/packages/adapter-utils/src/duplex-bridge-broker.ts @@ -258,6 +258,18 @@ function errorMessage(error: unknown): string { return error instanceof Error ? error.message : String(error); } +/** + * The safe HTTP methods. RFC 7231 section 4.2.1 defines this set. A safe method + * does not change host state, so the host applies no mutation for it. A caller + * can retry a safe method after a forward failure without a double-apply risk. + */ +const SAFE_BRIDGE_METHODS = new Set(["GET", "HEAD", "OPTIONS", "TRACE"]); + +/** Report whether the method is safe, so a forward failure stays retryable. */ +function isSafeBridgeMethod(method: string): boolean { + return SAFE_BRIDGE_METHODS.has(method.trim().toUpperCase()); +} + /** The internal bookkeeping for one in-flight request. */ interface PendingRequest { controller: AbortController; @@ -535,20 +547,46 @@ export function createDuplexBridgeBroker(options: DuplexBrokerOptions): DuplexBr ); return; } - // 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. + // The forward rejected before the host delivered a response. This + // rejection does not prove that the host applied no mutation. A fetch + // can reject after the request bytes reach the host and the host + // commits, but before the response headers arrive. A safe method never + // changes host state, so a retry stays safe for it. For any other + // method the host may have committed, so the outcome is indeterminate. + if (isSafeBridgeMethod(frame.method)) { + // The method is safe, so a retry cannot double-apply a mutation. + // Return a 502 with the completed outcome, so the gateway passes it + // through as a retryable status. + respond( + frame.id, + { + status: 502, + headers: { "content-type": "application/json" }, + body: JSON.stringify({ error: errorMessage(error) }), + }, + "completed", + "error", + ); + return; + } + // The method may mutate host state, so a retry with a new request id + // could apply the mutation twice. Return a non-retryable 504 and mark + // the outcome indeterminate, so the gateway maps it to a terminal 409. respond( frame.id, { - status: 502, - headers: { "content-type": "application/json" }, - body: JSON.stringify({ error: errorMessage(error) }), + status: 504, + headers: { + "content-type": "application/json", + "x-paperclip-bridge-outcome": "indeterminate", + }, + body: JSON.stringify({ + error: errorMessage(error), + outcome: "indeterminate", + retryable: false, + }), }, - "completed", + "indeterminate", "error", ); },