mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-27 02:44:50 +01:00
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 <noreply@paperclip.ing>
This commit is contained in:
3 files changed
+259
-9
No files matched your search
@@ -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,
|
||||
|
||||
@@ -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[]) {
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in new issue
Block a user