mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-21 00:05:49 +02:00
perf(gateway): remove the whole-body string round trip on the HTTP/2 send path (#12189)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Sandbox agents send callback requests through a generated gateway. > - The HTTP/2 callback path converts each full request body to a UTF-8 string. > - The same path converts that string back to a buffer before it sends the request. > - This pull request sends the body buffer directly to the HTTP/2 forwarder. > - The benefit is less copying and unchanged queue payload behavior. ## Linked Issues or Issue Description **What existing behavior does this improve?** The sandbox callback gateway HTTP/2 send path copies the full request body through a UTF-8 string before it sends the body. **Subsystem affected** The affected subsystem is `packages/adapter-utils`, which contains sandbox gateway and HTTP/2 adapter utilities. **Current behavior** The gateway reads the body into a buffer, converts the complete body to a UTF-8 string, and converts that string back to a buffer for the HTTP/2 path. The queue path stores the string payload. **Proposed behavior** The gateway keeps a byte reader for the HTTP/2 path. A thin text wrapper keeps the queue path behavior. The HTTP/2 path sends the original body buffer. **Reason and benefit** The extra conversions add work and memory use without changing the HTTP/2 body bytes. Direct buffer forwarding removes that work and preserves the size limit and reject behavior. **Breaking changes** None. The queue payload remains a string. The body size limit, content type check, and reject point remain unchanged. **Additional context** This change has no public issue link. The repository roadmap search found no duplicate planned work. The implementation also adds tests for non-ASCII JSON, malformed UTF-8, size limits, and queue payload shape. ## What Changed - Add `readBodyBytes(req)` for byte-preserving body reads. - Keep `readBody(req)` as a string wrapper for the queue path. - Send the byte buffer directly on the HTTP/2 path. - Add tests for byte identity, malformed UTF-8, size limits, and queue payload shape. ## Verification - `pnpm --filter @paperclip/adapter-utils test src/sandbox-callback-bridge.test.ts` passed with 51 tests. - `pnpm --filter @paperclip/adapter-utils exec tsc --noEmit` passed. - The tests spawn the generated gateway and the real host HTTP/2 bridge. - The tests verify byte identity, malformed UTF-8, pre-forward size rejection, queue file protection, and string queue payloads. - Full repository CI must pass before merge. ## Risks Low risk. The HTTP/2 path changes its internal body conversion only. The queue path keeps the prior string payload. The size limit and reject point stay unchanged. ## Model Used OpenAI Codex, GPT-5, tool use and code review assistance. The exact runtime context window is managed by the Codex service. ## 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 <noreply@paperclip.ing>
This commit is contained in:
1 parent
0cedb45df3
commit
29d12045f4
2 files changed
+357
-3
No files matched your search
@@ -22,6 +22,9 @@ import {
|
||||
startSandboxCallbackBridgeWorker,
|
||||
} from "./sandbox-callback-bridge.js";
|
||||
import type { SandboxCallbackBridgeQueueClient } from "./sandbox-callback-bridge.js";
|
||||
import { createHttp2BridgeServer } from "./http2-bridge-server.js";
|
||||
import type { Http2BridgeForwardRequest, Http2BridgeForwardResult } from "./http2-bridge-server.js";
|
||||
import type { CommandManagedDuplexChannel } from "./command-managed-runtime.js";
|
||||
import type { RuntimeSpanRunner } from "./acpx-engine/startup-timing.js";
|
||||
import type { RunProcessResult } from "./server-utils.js";
|
||||
|
||||
@@ -3084,4 +3087,351 @@ describe("sandbox callback bridge", () => {
|
||||
const afterReady = firstBytes.subarray(newlineIndex + 1, newlineIndex + 1 + preface.length);
|
||||
expect(afterReady).toEqual(preface);
|
||||
}, 15_000);
|
||||
|
||||
/**
|
||||
* Spawn the real generated gateway in `http2_v1` mode, then bind the real
|
||||
* host-side `createHttp2BridgeServer` to its stdio. The gateway writes one
|
||||
* READY line before it hands stdout to its HTTP/2 client, so this helper
|
||||
* strips that line first and feeds the host only the raw HTTP/2 bytes that
|
||||
* follow. A test then drives the gateway with ordinary HTTP/1.1 requests
|
||||
* against its loopback port and inspects the exact bytes `forwardRequest`
|
||||
* receives, proving the send path carries no intermediate string.
|
||||
*/
|
||||
async function startHttp2GatewayForTest(options: {
|
||||
bridgeToken: string;
|
||||
maxBodyBytes?: number;
|
||||
forwardRequest: (request: Http2BridgeForwardRequest) => Promise<Http2BridgeForwardResult>;
|
||||
}): Promise<{ baseUrl: string; stop: () => Promise<void> }> {
|
||||
const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-bridge-http2-test-"));
|
||||
cleanupDirs.push(rootDir);
|
||||
const entrypoint = path.join(rootDir, "paperclip-bridge-server.mjs");
|
||||
await writeFile(entrypoint, getSandboxCallbackBridgeServerSource(), "utf8");
|
||||
|
||||
const probe = createServer();
|
||||
const assignedPort = await new Promise<number>((resolve, reject) => {
|
||||
probe.once("error", reject);
|
||||
probe.listen(0, "127.0.0.1", () => {
|
||||
const address = probe.address();
|
||||
if (!address || typeof address === "string") {
|
||||
reject(new Error("Could not reserve a loopback port for the test."));
|
||||
return;
|
||||
}
|
||||
probe.close(() => resolve(address.port));
|
||||
});
|
||||
});
|
||||
|
||||
const child = spawn(process.execPath, [entrypoint], {
|
||||
env: {
|
||||
...process.env,
|
||||
PAPERCLIP_API_BRIDGE_MODE: "http2_v1",
|
||||
PAPERCLIP_BRIDGE_TOKEN: options.bridgeToken,
|
||||
PAPERCLIP_BRIDGE_PORT: String(assignedPort),
|
||||
PAPERCLIP_BRIDGE_NONCE: "test-nonce",
|
||||
...(options.maxBodyBytes != null
|
||||
? { PAPERCLIP_BRIDGE_MAX_BODY_BYTES: String(options.maxBodyBytes) }
|
||||
: {}),
|
||||
},
|
||||
stdio: ["pipe", "pipe", "pipe"],
|
||||
});
|
||||
let stderr = "";
|
||||
child.stderr.on("data", (chunk: Buffer) => {
|
||||
stderr += chunk.toString("utf8");
|
||||
});
|
||||
|
||||
const dataListeners: Array<(chunk: Uint8Array) => void> = [];
|
||||
const pending: Buffer[] = [];
|
||||
let dispatchStarted = false;
|
||||
let sawReadyLine = false;
|
||||
let readyBuffer = Buffer.alloc(0);
|
||||
let resolveReady!: () => void;
|
||||
let rejectReady!: (error: Error) => void;
|
||||
const readyPromise = new Promise<void>((resolve, reject) => {
|
||||
resolveReady = resolve;
|
||||
rejectReady = reject;
|
||||
});
|
||||
const readyTimer = setTimeout(
|
||||
() => rejectReady(new Error("Timed out waiting for the http2 gateway READY line. stderr: " + stderr)),
|
||||
5000,
|
||||
);
|
||||
const deliver = (chunk: Buffer) => {
|
||||
if (dispatchStarted) {
|
||||
for (const listener of dataListeners) listener(chunk);
|
||||
} else {
|
||||
pending.push(chunk);
|
||||
}
|
||||
};
|
||||
child.stdout.on("data", (chunk: Buffer) => {
|
||||
if (!sawReadyLine) {
|
||||
readyBuffer = Buffer.concat([readyBuffer, chunk]);
|
||||
const newlineIndex = readyBuffer.indexOf(0x0a);
|
||||
if (newlineIndex === -1) return;
|
||||
sawReadyLine = true;
|
||||
clearTimeout(readyTimer);
|
||||
const rest = readyBuffer.subarray(newlineIndex + 1);
|
||||
readyBuffer = Buffer.alloc(0);
|
||||
resolveReady();
|
||||
if (rest.length > 0) deliver(rest);
|
||||
return;
|
||||
}
|
||||
deliver(chunk);
|
||||
});
|
||||
child.once("exit", (code) => {
|
||||
clearTimeout(readyTimer);
|
||||
if (!sawReadyLine) {
|
||||
rejectReady(new Error("The http2 gateway exited early with code " + String(code) + ". stderr: " + stderr));
|
||||
}
|
||||
});
|
||||
|
||||
const channel: CommandManagedDuplexChannel = {
|
||||
write: (data) => {
|
||||
child.stdin.write(Buffer.from(data));
|
||||
},
|
||||
onData: (listener) => {
|
||||
dataListeners.push(listener);
|
||||
},
|
||||
onExit: (listener) => {
|
||||
child.once("exit", (code) => listener({ exitCode: code }));
|
||||
},
|
||||
stop: () => {
|
||||
child.kill();
|
||||
},
|
||||
close: async () => {
|
||||
child.stdin.end();
|
||||
},
|
||||
};
|
||||
|
||||
await readyPromise;
|
||||
|
||||
const handle = createHttp2BridgeServer({
|
||||
bridgeToken: options.bridgeToken,
|
||||
forwardRequest: options.forwardRequest,
|
||||
});
|
||||
const boundDuplex = handle.bindChannel(channel);
|
||||
dispatchStarted = true;
|
||||
const buffered = pending.splice(0);
|
||||
for (const chunk of buffered) {
|
||||
for (const listener of dataListeners) listener(chunk);
|
||||
}
|
||||
|
||||
// Destroy the bound duplex directly instead of `handle.close()`. The
|
||||
// duplex here wraps a spawned process's raw stdio, not a real socket,
|
||||
// and `close()` waits on a graceful HTTP/2 GOAWAY exchange that never
|
||||
// settles over this transport. A direct destroy ends the session at
|
||||
// once, which is correct for test teardown.
|
||||
const stop = async () => {
|
||||
boundDuplex.destroy();
|
||||
child.kill();
|
||||
};
|
||||
cleanupFns.push(stop);
|
||||
|
||||
return { baseUrl: `http://127.0.0.1:${assignedPort}`, stop };
|
||||
}
|
||||
|
||||
it("forwards the exact request body bytes to the HTTP/2 host handler, including a non-ASCII character", async () => {
|
||||
const bridgeToken = createSandboxCallbackBridgeToken();
|
||||
const seenBodies: Buffer[] = [];
|
||||
const gateway = await startHttp2GatewayForTest({
|
||||
bridgeToken,
|
||||
forwardRequest: async (request) => {
|
||||
seenBodies.push(request.body);
|
||||
return { status: 200, headers: { "content-type": "application/json" }, body: JSON.stringify({ ok: true }) };
|
||||
},
|
||||
});
|
||||
|
||||
// "café" holds one multi-byte UTF-8 character. A body-to-string-to-body
|
||||
// round trip still reproduces this text correctly, so the byte-for-byte
|
||||
// comparison below is the real proof: it fails if any re-encoding step
|
||||
// runs, even one that happens to preserve valid UTF-8 text.
|
||||
const bodyText = JSON.stringify({ note: "café" });
|
||||
const bodyBytes = Buffer.from(bodyText, "utf8");
|
||||
const response = await fetch(`${gateway.baseUrl}/api/issues/issue-1/comments`, {
|
||||
method: "POST",
|
||||
headers: {
|
||||
authorization: `Bearer ${bridgeToken}`,
|
||||
"content-type": "application/json",
|
||||
},
|
||||
body: bodyBytes,
|
||||
});
|
||||
expect(response.status).toBe(200);
|
||||
expect(seenBodies).toHaveLength(1);
|
||||
expect(seenBodies[0]?.equals(bodyBytes)).toBe(true);
|
||||
}, 15_000);
|
||||
|
||||
it("forwards malformed UTF-8 bytes to the HTTP/2 host handler unchanged", async () => {
|
||||
const bridgeToken = createSandboxCallbackBridgeToken();
|
||||
const seenBodies: Buffer[] = [];
|
||||
const gateway = await startHttp2GatewayForTest({
|
||||
bridgeToken,
|
||||
forwardRequest: async (request) => {
|
||||
seenBodies.push(request.body);
|
||||
return { status: 200, headers: {}, body: "" };
|
||||
},
|
||||
});
|
||||
|
||||
// Byte 0xC3 opens a two-byte UTF-8 sequence; 0x28 is not a valid
|
||||
// continuation byte, so this body is not valid UTF-8. The gateway does
|
||||
// not decode or validate the body, so these exact bytes must still
|
||||
// arrive at the host handler unchanged.
|
||||
const malformedBytes = Buffer.from([0x7b, 0x22, 0x61, 0x22, 0x3a, 0xc3, 0x28, 0x7d]);
|
||||
const response = await fetch(`${gateway.baseUrl}/api/issues/issue-1/comments`, {
|
||||
method: "POST",
|
||||
headers: {
|
||||
authorization: `Bearer ${bridgeToken}`,
|
||||
"content-type": "application/json",
|
||||
},
|
||||
body: malformedBytes,
|
||||
});
|
||||
expect(response.status).toBe(200);
|
||||
expect(seenBodies).toHaveLength(1);
|
||||
expect(seenBodies[0]?.equals(malformedBytes)).toBe(true);
|
||||
}, 15_000);
|
||||
|
||||
it("rejects a request body over maxBodyBytes on the HTTP/2 path before it forwards a byte", async () => {
|
||||
const bridgeToken = createSandboxCallbackBridgeToken();
|
||||
const maxBodyBytes = 32;
|
||||
let forwardCalls = 0;
|
||||
const gateway = await startHttp2GatewayForTest({
|
||||
bridgeToken,
|
||||
maxBodyBytes,
|
||||
forwardRequest: async () => {
|
||||
forwardCalls += 1;
|
||||
return { status: 200, headers: {}, body: "" };
|
||||
},
|
||||
});
|
||||
|
||||
const oversizeBody = Buffer.alloc(maxBodyBytes + 1, 0x41);
|
||||
const response = await fetch(`${gateway.baseUrl}/api/issues/issue-1/comments`, {
|
||||
method: "POST",
|
||||
headers: {
|
||||
authorization: `Bearer ${bridgeToken}`,
|
||||
"content-type": "application/json",
|
||||
},
|
||||
body: oversizeBody,
|
||||
});
|
||||
expect(response.status).toBe(502);
|
||||
await expect(response.json()).resolves.toMatchObject({
|
||||
error: "Bridge request body exceeded the configured size limit.",
|
||||
});
|
||||
expect(forwardCalls).toBe(0);
|
||||
}, 15_000);
|
||||
|
||||
it("rejects a request body over maxBodyBytes on the queue path before it writes the queue file", async () => {
|
||||
const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-bridge-queue-maxbody-"));
|
||||
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 maxBodyBytes 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 directories = sandboxCallbackBridgeDirectories(queueDir);
|
||||
const bridgeToken = createSandboxCallbackBridgeToken();
|
||||
const maxBodyBytes = 32;
|
||||
|
||||
const bridge = await startSandboxCallbackBridgeServer({
|
||||
runner,
|
||||
remoteCwd: remoteWorkspaceDir,
|
||||
assetRemoteDir: prepared.assetDirs.bridge,
|
||||
queueDir,
|
||||
bridgeToken,
|
||||
timeoutMs: 30_000,
|
||||
maxBodyBytes,
|
||||
});
|
||||
cleanupFns.push(async () => {
|
||||
await bridge.stop();
|
||||
});
|
||||
|
||||
const oversizeBody = Buffer.alloc(maxBodyBytes + 1, 0x41);
|
||||
const response = await fetch(`${bridge.baseUrl}/api/issues/issue-1/comments`, {
|
||||
method: "POST",
|
||||
headers: {
|
||||
authorization: `Bearer ${bridgeToken}`,
|
||||
"content-type": "application/json",
|
||||
},
|
||||
body: oversizeBody,
|
||||
});
|
||||
expect(response.status).toBe(502);
|
||||
await expect(response.json()).resolves.toMatchObject({
|
||||
error: "Bridge request body exceeded the configured size limit.",
|
||||
});
|
||||
|
||||
const requestFiles = await readdir(directories.requestsDir);
|
||||
expect(requestFiles.filter((name) => name.endsWith(".json"))).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("keeps the queue request payload's body field as a plain JSON string", async () => {
|
||||
const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-bridge-queue-body-shape-"));
|
||||
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 body shape 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 requestBodyText = JSON.stringify({ note: "café" });
|
||||
|
||||
const seenRequests: Array<{ body: string }> = [];
|
||||
const worker = await startSandboxCallbackBridgeWorker({
|
||||
client: createFileSystemSandboxCallbackBridgeQueueClient(),
|
||||
queueDir,
|
||||
authorizeRequest: async () => null,
|
||||
handleRequest: async (request) => {
|
||||
seenRequests.push({ body: request.body });
|
||||
return { status: 200, headers: {}, body: JSON.stringify({ ok: true }) };
|
||||
},
|
||||
});
|
||||
cleanupFns.push(async () => {
|
||||
await worker.stop();
|
||||
});
|
||||
|
||||
const bridge = await startSandboxCallbackBridgeServer({
|
||||
runner,
|
||||
remoteCwd: remoteWorkspaceDir,
|
||||
assetRemoteDir: prepared.assetDirs.bridge,
|
||||
queueDir,
|
||||
bridgeToken,
|
||||
timeoutMs: 30_000,
|
||||
});
|
||||
cleanupFns.push(async () => {
|
||||
await bridge.stop();
|
||||
});
|
||||
|
||||
const response = await fetch(`${bridge.baseUrl}/api/issues/issue-1/comments`, {
|
||||
method: "POST",
|
||||
headers: {
|
||||
authorization: `Bearer ${bridgeToken}`,
|
||||
"content-type": "application/json",
|
||||
},
|
||||
body: requestBodyText,
|
||||
});
|
||||
expect(response.status).toBe(200);
|
||||
expect(seenRequests).toHaveLength(1);
|
||||
expect(typeof seenRequests[0]?.body).toBe("string");
|
||||
expect(seenRequests[0]?.body).toBe(requestBodyText);
|
||||
});
|
||||
});
|
||||
@@ -2133,7 +2133,7 @@ function normalizeHeaders(headers) {
|
||||
return out;
|
||||
}
|
||||
|
||||
async function readBody(req) {
|
||||
async function readBodyBytes(req) {
|
||||
const chunks = [];
|
||||
let totalBytes = 0;
|
||||
for await (const chunk of req) {
|
||||
@@ -2144,7 +2144,11 @@ async function readBody(req) {
|
||||
throw new Error("Bridge request body exceeded the configured size limit.");
|
||||
}
|
||||
}
|
||||
return Buffer.concat(chunks).toString("utf8");
|
||||
return Buffer.concat(chunks);
|
||||
}
|
||||
|
||||
async function readBody(req) {
|
||||
return (await readBodyBytes(req)).toString("utf8");
|
||||
}
|
||||
|
||||
function tokensMatch(received) {
|
||||
@@ -2475,7 +2479,7 @@ function runHttp2Gateway() {
|
||||
writeJsonResponse(res, 415, { error: "Bridge only accepts JSON request bodies." });
|
||||
return;
|
||||
}
|
||||
const requestBodyBuffer = Buffer.from(await readBody(req), "utf8");
|
||||
const requestBodyBuffer = await readBodyBytes(req);
|
||||
let response;
|
||||
try {
|
||||
response = await forwardOverHttp2({
|
||||
|
||||
Reference in new issue
Block a user