From 84f3a8eaf944ae9e8244f33774806bccdd89c501 Mon Sep 17 00:00:00 2001 From: Priya Raman Date: Sun, 23 Aug 2026 21:23:34 +0000 Subject: [PATCH] fix(adapter-utils): charge a bodyless request the zero-length reserve, not the worst-case cap A request with no Content-Length header and no Transfer-Encoding header has no body (RFC 9112 section 6.3). declaredBodyBytes returned null for that shape, so both bridge gateways charged the same worst-case maxBodyBytes reserve as an unknown-size chunked body. A plain DELETE takes this exact shape, so two concurrent DELETE requests -- or one DELETE next to one large POST -- exceeded the production inflight bound and got a 503, even though neither request holds any real body memory. Classify the framing in three ways instead of two: a Transfer-Encoding header always means an unknown length; a valid Content-Length header is a declared length; neither header present is a declared length of zero. Both gateways already treat a non-null declared length as the real charge, so this alone fixes the reserve for both gateways with no other call-site change. The zero case still reads through readBody with a declared length of zero, so an unexpected body byte still raises a 413. The charge for a declared length and for an unknown (chunked) length is unchanged in value. The chunked worst case is documented in the comment on the charge: at production defaults the file gateway admits exactly one such request at a time, and this is intentional -- the gateway cannot know a chunked body's size before it reads it. Regression tests prove two concurrent DELETE requests, and one DELETE concurrent with a POST at the body cap, both reach the host worker at production admission defaults; they fail against the prior commit and pass after this fix. A duplex-gateway test proves the same zero-length charge with the duplex charge model unchanged for declared and unknown lengths. Co-authored-by: Paperclip --- .../src/duplex-bridge-local-e2e.test.ts | 33 ++++ .../src/sandbox-callback-bridge.test.ts | 168 +++++++++++++++++- .../src/sandbox-callback-bridge.ts | 67 ++++++- 3 files changed, 259 insertions(+), 9 deletions(-) diff --git a/packages/adapter-utils/src/duplex-bridge-local-e2e.test.ts b/packages/adapter-utils/src/duplex-bridge-local-e2e.test.ts index c26350ed848..5c0fa464417 100644 --- a/packages/adapter-utils/src/duplex-bridge-local-e2e.test.ts +++ b/packages/adapter-utils/src/duplex-bridge-local-e2e.test.ts @@ -889,6 +889,39 @@ describe("duplex bridge local end-to-end harness", () => { expect(later.status).toBe(200); }, 20000); + it("charges a bodyless DELETE the zero-length reserve, not the worst-case maxBodyBytes charge", async () => { + // Regression coverage for PAP-5046. A DELETE with no body sends neither a + // Content-Length header nor a Transfer-Encoding header. Before the fix, + // declaredBodyBytes returned null for that shape and the gateway charged + // the worst-case maxBodyBytes reserve, the same charge as an unknown-size + // chunked body. This pins maxInflightBodyBytes to fit the zero-length + // charge (0 + DEFAULT_MAX_DUPLEX_FRAME_BYTES) but not the worst-case + // charge (maxBodyBytes + DEFAULT_MAX_DUPLEX_FRAME_BYTES), so only the + // fixed classification admits the request. + const maxBodyBytes = 4096; + const maxInflightBodyBytes = DEFAULT_MAX_DUPLEX_FRAME_BYTES; + const harness = await createHarness({ maxBodyBytes, maxInflightBodyBytes }); + harnesses.push(harness); + await harness.waitFor(readyPredicate(harness), "the gateway to become ready"); + + let apiWasCalled = false; + harness.api.setResponder((_req, res) => { + apiWasCalled = true; + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify({ ok: true })); + }); + + // /api/routine-triggers/{id} is the one DELETE route the host allowlist + // admits; a DELETE elsewhere gets a 403 before it reaches the API, + // regardless of this fix, so this path isolates the admission charge. + const response = await fetch(`${harness.baseUrl}/api/routine-triggers/t-1`, { + method: "DELETE", + headers: { authorization: `Bearer ${harness.bridgeToken}`, "content-type": "application/json" }, + }); + expect(response.status).not.toBe(503); + expect(apiWasCalled).toBe(true); + }, 20000); + it("admits many small concurrent requests that the old flat 10 MiB charge would have shed with 503", async () => { // Production constants. Under the old flat charge every active read // reserved maxBodyBytes (about 10 MiB) regardless of the real body size, diff --git a/packages/adapter-utils/src/sandbox-callback-bridge.test.ts b/packages/adapter-utils/src/sandbox-callback-bridge.test.ts index e9477c1733c..f73e8fc8247 100644 --- a/packages/adapter-utils/src/sandbox-callback-bridge.test.ts +++ b/packages/adapter-utils/src/sandbox-callback-bridge.test.ts @@ -1344,6 +1344,164 @@ describe("sandbox callback bridge", () => { expect(seenMethods).toEqual(["DELETE", "POST", "POST"]); }, 20000); + it("admits two concurrent DELETE requests with no body at production admission defaults", async () => { + // Regression coverage for PAP-5046. A DELETE with no body sends neither a + // Content-Length header nor a Transfer-Encoding header, so before the fix + // declaredBodyBytes returned null for it and the gateway charged the + // worst-case maxBodyBytes reserve, the same charge as an actual 10 MiB + // body. Two such requests together exceeded the production inflight + // bound and the second one got a 503. This test sends two concurrent + // DELETE requests at the production maxBodyBytes and maxInflightBodyBytes + // defaults (no override) and asserts both reach the host worker. + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-bridge-delete-concurrent-")); + cleanupDirs.push(rootDir); + + const localWorkspaceDir = path.join(rootDir, "local-workspace"); + const remoteWorkspaceDir = path.join(rootDir, "remote-workspace"); + await mkdir(localWorkspaceDir, { recursive: true }); + await mkdir(remoteWorkspaceDir, { recursive: true }); + await writeFile(path.join(localWorkspaceDir, "README.md"), "bridge delete concurrent test\n", "utf8"); + + const runner = createExecRunner(); + const bridgeAsset = await createSandboxCallbackBridgeAsset(); + cleanupFns.push(bridgeAsset.cleanup); + const prepared = await prepareCommandManagedRuntime({ + runner, + spec: { remoteCwd: remoteWorkspaceDir, timeoutMs: 30_000 }, + adapterKey: "codex", + workspaceLocalDir: localWorkspaceDir, + assets: [{ key: "bridge", localDir: bridgeAsset.localDir }], + }); + + const queueDir = path.posix.join(prepared.runtimeRootDir, "paperclip-bridge"); + const bridgeToken = createSandboxCallbackBridgeToken(); + + const seenMethods: string[] = []; + const worker = await startSandboxCallbackBridgeWorker({ + client: createFileSystemSandboxCallbackBridgeQueueClient(), + queueDir, + authorizeRequest: async () => null, + handleRequest: async (request) => { + seenMethods.push(request.method); + return { + status: 200, + headers: { "content-type": "application/json" }, + body: JSON.stringify({ ok: true }), + }; + }, + }); + cleanupFns.push(async () => { + await worker.stop(); + }); + + // No maxBodyBytes or maxInflightBodyBytes override -- both stay at their + // production defaults (10,485,761 and 67,108,864 bytes). + const bridge = await startSandboxCallbackBridgeServer({ + runner, + remoteCwd: remoteWorkspaceDir, + assetRemoteDir: prepared.assetDirs.bridge, + queueDir, + bridgeToken, + timeoutMs: 30_000, + pollIntervalMs: 10, + }); + cleanupFns.push(async () => { + await bridge.stop(); + }); + + const del = () => + fetch(`${bridge.baseUrl}/api/issues/issue-1`, { + method: "DELETE", + headers: { authorization: `Bearer ${bridgeToken}`, "content-type": "application/json" }, + }); + const [first, second] = await Promise.all([del(), del()]); + expect(first.status).not.toBe(503); + expect(second.status).not.toBe(503); + expect(seenMethods).toEqual(["DELETE", "DELETE"]); + }, 20000); + + it("admits a DELETE concurrent with a file-gateway POST at the production max-body-bytes bound", async () => { + // Regression coverage for PAP-5046. Before the fix, a bodyless DELETE + // charged the same worst-case reserve as a POST at the body cap. Two such + // charges together exceeded the production inflight bound, so a legal + // DELETE competing with a legal large POST got shed with a 503. This test + // sends one DELETE concurrent with one POST at exactly one byte under the + // production body cap (10,485,760 bytes), with no maxBodyBytes or + // maxInflightBodyBytes override, and asserts both reach the host worker. + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-bridge-delete-post-concurrent-")); + cleanupDirs.push(rootDir); + + const localWorkspaceDir = path.join(rootDir, "local-workspace"); + const remoteWorkspaceDir = path.join(rootDir, "remote-workspace"); + await mkdir(localWorkspaceDir, { recursive: true }); + await mkdir(remoteWorkspaceDir, { recursive: true }); + await writeFile(path.join(localWorkspaceDir, "README.md"), "bridge delete+post concurrent test\n", "utf8"); + + const runner = createExecRunner(); + const bridgeAsset = await createSandboxCallbackBridgeAsset(); + cleanupFns.push(bridgeAsset.cleanup); + const prepared = await prepareCommandManagedRuntime({ + runner, + spec: { remoteCwd: remoteWorkspaceDir, timeoutMs: 30_000 }, + adapterKey: "codex", + workspaceLocalDir: localWorkspaceDir, + assets: [{ key: "bridge", localDir: bridgeAsset.localDir }], + }); + + const queueDir = path.posix.join(prepared.runtimeRootDir, "paperclip-bridge"); + const bridgeToken = createSandboxCallbackBridgeToken(); + + const seenMethods: string[] = []; + const worker = await startSandboxCallbackBridgeWorker({ + client: createFileSystemSandboxCallbackBridgeQueueClient(), + queueDir, + authorizeRequest: async () => null, + handleRequest: async (request) => { + seenMethods.push(request.method); + return { + status: 200, + headers: { "content-type": "application/json" }, + body: JSON.stringify({ ok: true }), + }; + }, + }); + cleanupFns.push(async () => { + await worker.stop(); + }); + + // No maxBodyBytes or maxInflightBodyBytes override -- both stay at their + // production defaults (10,485,761 and 67,108,864 bytes). + const bridge = await startSandboxCallbackBridgeServer({ + runner, + remoteCwd: remoteWorkspaceDir, + assetRemoteDir: prepared.assetDirs.bridge, + queueDir, + bridgeToken, + timeoutMs: 30_000, + pollIntervalMs: 10, + }); + cleanupFns.push(async () => { + await bridge.stop(); + }); + + const del = () => + fetch(`${bridge.baseUrl}/api/issues/issue-1`, { + method: "DELETE", + headers: { authorization: `Bearer ${bridgeToken}`, "content-type": "application/json" }, + }); + const largeBody = "a".repeat(10 * 1024 * 1024 - 1); + const post = () => + fetch(`${bridge.baseUrl}/api/issues/issue-1/comments`, { + method: "POST", + headers: { authorization: `Bearer ${bridgeToken}`, "content-type": "application/json" }, + body: largeBody, + }); + const [delResponse, postResponse] = await Promise.all([del(), post()]); + expect(delResponse.status).not.toBe(503); + expect(postResponse.status).not.toBe(503); + expect(seenMethods.sort()).toEqual(["DELETE", "POST"]); + }, 30000); + it("answers a non-retryable 413, not a retryable 503, when a single read's charge alone can never fit the inflight bound", async () => { // Regression coverage for the PAP-5041 scope amendment's criterion 9: // reserveBodyRead must not answer a retryable 503 for a read that can @@ -1522,8 +1680,16 @@ describe("sandbox callback bridge", () => { }; const embedded = factory(); - expect(embedded.declaredBodyBytes({ headers: {} })).toBeNull(); + // PAP-5046: a request with neither header has no body (RFC 9112 section + // 6.3), so it classifies as a declared zero, not an unknown length. + expect(embedded.declaredBodyBytes({ headers: {} })).toBe(0); expect(embedded.declaredBodyBytes({ headers: { "content-length": "42" } })).toBe(42); + // A Transfer-Encoding header always means an unknown length, even when a + // Content-Length header rides alongside it. + expect(embedded.declaredBodyBytes({ headers: { "transfer-encoding": "chunked" } })).toBeNull(); + expect( + embedded.declaredBodyBytes({ headers: { "content-length": "42", "transfer-encoding": "chunked" } }), + ).toBeNull(); const maxBodyBytes = 10 * 1024 * 1024 + 1; async function* fakeRequest(chunks: Buffer[]) { diff --git a/packages/adapter-utils/src/sandbox-callback-bridge.ts b/packages/adapter-utils/src/sandbox-callback-bridge.ts index 8cac187a777..9080a9b8601 100644 --- a/packages/adapter-utils/src/sandbox-callback-bridge.ts +++ b/packages/adapter-utils/src/sandbox-callback-bridge.ts @@ -2130,15 +2130,40 @@ class BridgeBodyTooLargeError extends Error { } } -// Read the declared body size from the Content-Length request header. Node's -// HTTP server validates the header's syntax before a request reaches a -// handler, so this only needs to guard the two cases that leave the gateway -// with no declared size: the header is absent, or its value is not a plain -// non-negative integer (a chunked request carries no Content-Length header -// at all). Both cases return null, so a caller can tell a declared length -// from an unknown one and pick the right read strategy in readBody. +// Classify a request's body framing into exactly three states, so a caller +// charges the right admission reserve for each one instead of folding +// "no body at all" into the same worst-case charge as "a body of unknown +// size". This function applies RFC 9112 section 6.3's precedence: +// +// 1. A Transfer-Encoding header is present: the length is unknown, even +// when a Content-Length header rides alongside it. Chunked framing +// wins, and the worst-case charge is the safe answer, because the +// gateway cannot know a chunked body's size before it reads it. +// 2. No Transfer-Encoding header, and a valid Content-Length header (a +// plain non-negative safe integer): the length is declared, and it is +// that value. +// 3. Neither header is present: RFC 9112 section 6.3 says a request with +// no Content-Length header and no Transfer-Encoding header has no +// body, so the declared length is zero. A DELETE request commonly +// takes this form. +// +// Returns a non-negative integer for a declared length (0 for a bodyless +// request), or null for an unknown length. A caller that sees null still +// substitutes the worst-case maxBodyBytes charge, exactly as before; a +// caller that sees 0 now charges the zero-length reserve instead. Node's +// HTTP server validates each header's syntax before a request reaches a +// handler, so a malformed Content-Length value only reaches this function +// through Node's own escape hatch; this function treats that case as +// unknown, the same safe answer Node's own rejection would have forced. function declaredBodyBytes(req) { + const transferEncoding = req.headers["transfer-encoding"]; + if (typeof transferEncoding === "string" && transferEncoding.length > 0) { + return null; + } const raw = req.headers["content-length"]; + if (raw === undefined) { + return 0; + } if (typeof raw !== "string" || !/^[0-9]+$/.test(raw)) { return null; } @@ -2578,6 +2603,19 @@ async function runFileGateway() { // The charge substitutes maxBodyBytes for an unknown declared length -- // the same worst-case charge the old flat model always used -- but // readBody itself never preallocates that worst case; see its comment. + // declaredBodyBytes now returns 0, not null, for a request with + // neither a Content-Length nor a Transfer-Encoding header, so a + // bodyless request (a DELETE, for example) charges the zero-length + // reserve here, not this worst-case fallback. + // + // A chunked request still declares no length (declaredBodyBytes + // returns null for it), so it still charges the whole cap here. At + // the production defaults (maxBodyBytes 10,485,761, factor 4, extra + // bytes 393,216) that charge is about 42.3 MB, and the 64 MiB + // inflight bound admits exactly one such request at a time. This is + // intentional: the gateway cannot know a chunked body's size before + // it reads it, so an honest worst-case charge is the correct, safe + // answer. const reserveCharge = hasRequestBody ? (declaredLength !== null ? declaredLength : maxBodyBytes) : 0; const reserved = hasRequestBody ? reserveBodyRead(reserveCharge * FILE_GATEWAY_BODY_RESERVE_FACTOR + FILE_GATEWAY_BODY_RESERVE_EXTRA_BYTES) @@ -2948,7 +2986,20 @@ function runDuplexGateway() { } // The charge substitutes maxBodyBytes for an unknown declared length -- // the same worst-case charge as before -- but readBody itself never - // preallocates that worst case; see its comment. + // preallocates that worst case; see its comment. declaredBodyBytes now + // returns 0, not null, for a request with neither a Content-Length nor + // a Transfer-Encoding header, so a bodyless request charges the + // zero-length reserve here, not this worst-case fallback. The charge + // for a declared length and for an unknown length is unchanged in + // value from before. + // + // A chunked request still declares no length (declaredBodyBytes + // returns null for it), so it still charges the whole cap here. At + // the production defaults (maxBodyBytes 10,485,761, extra bytes + // 1,048,576) that charge is about 11.5 MB, and the 64 MiB inflight + // bound admits up to 5 such requests at a time. This is intentional: + // the gateway cannot know a chunked body's size before it reads it, + // so an honest worst-case charge is the correct, safe answer. const reserveCharge = hasRequestBody ? (declaredLength !== null ? declaredLength : maxBodyBytes) : 0; const reserved = hasRequestBody ? reserveBodyRead(reserveCharge + DUPLEX_GATEWAY_BODY_READ_RESERVE_EXTRA_BYTES)