From 5e6ac7dbcf76d4e3c06041f341b2c585d5ef69fd Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 6 Jul 2026 13:36:33 +0000 Subject: [PATCH 1/2] s3: keep response metadata across progress callbacks The HTTP client hands `HTTPResponseMetadata` to a task exactly once, on the first progress callback that carries response headers (`to_result()` does `state.cloned_metadata.take()`). A response body delimited by connection close (no Content-Length, no Transfer-Encoding) reports progress once with the headers and again at EOF, and the second callback carries no metadata. `S3HttpSimpleTask::http_callback` overwrote `this.result` wholesale on every callback, dropping the stored metadata, so `on_response` found `None` and unwrapped it, aborting the process on a legal HTTP/1.1 200. Carry the earlier metadata across the assignment, matching what `install::NetworkTask::notify` and `FetchTasklet::callback` already do. --- src/runtime/webcore/s3/simple_request.rs | 7 ++ test/js/bun/s3/s3-connection-close.test.ts | 115 +++++++++++++++++++-- 2 files changed, 113 insertions(+), 9 deletions(-) diff --git a/src/runtime/webcore/s3/simple_request.rs b/src/runtime/webcore/s3/simple_request.rs index 986715dbe109..b4114d0536f4 100644 --- a/src/runtime/webcore/s3/simple_request.rs +++ b/src/runtime/webcore/s3/simple_request.rs @@ -454,9 +454,16 @@ impl S3HttpSimpleTask { // HTTP thread until enqueued back to the JS thread below. let this = unsafe { &mut *this }; let is_done = !result.has_more; + // `metadata` is handed over exactly once, on the first callback carrying response headers. + // A close-delimited body (no Content-Length, no Transfer-Encoding) reports progress again + // at EOF with `metadata: None`, so carry the earlier one across the assignment below. + let previous_metadata = this.result.metadata.take(); // SAFETY: `result.body` (the only borrowed field) points at `this.response_buffer`, which // lives for the task's lifetime — extending to `'static` here is sound for self-reference. this.result = unsafe { result.detach_lifetime() }; + if this.result.metadata.is_none() { + this.result.metadata = previous_metadata; + } // `AsyncHTTP` transitively owns Drop types (`HTTPClient`, header // `EntryList`s), so a plain `=` here would (a) drop the old `this.http`, freeing heap // buffers that `*async_http` (a bitwise clone created by the HTTP thread) still aliases, diff --git a/test/js/bun/s3/s3-connection-close.test.ts b/test/js/bun/s3/s3-connection-close.test.ts index 184a9fc1e7fb..31e1cac5aaf6 100644 --- a/test/js/bun/s3/s3-connection-close.test.ts +++ b/test/js/bun/s3/s3-connection-close.test.ts @@ -36,6 +36,86 @@ process.exit(0); `; } +// A success response carrying a body but neither Content-Length nor Transfer-Encoding +// is delimited by the connection close (RFC 9112 section 6.3, rule 8) — what HTTP/1.0 +// upstreams and framing-stripping reverse proxies in front of S3 produce. Its headers +// arrive on one progress update and the completion on a later one carrying no metadata. +const SMALL_BODY = "hello-close-delimited"; +// Several times one TCP segment, so the body spans several progress updates and the +// headers end up many callbacks behind the completion. +const LARGE_BODY_BYTES = 256 * 1024; + +// Distinct first and last bytes so a body truncated, duplicated, or reassembled out of +// order across progress updates fails the assertion. Mirrored by `MAKE_LARGE_BODY` in +// the fixture; the head/tail/length assertion catches any drift between the two. +function largeBody() { + const body = Buffer.alloc(LARGE_BODY_BYTES, "x"); + body.write("START"); + body.write("END", LARGE_BODY_BYTES - 3); + return body.toString(); +} + +const MAKE_LARGE_BODY = `(() => { + const b = Buffer.alloc(${LARGE_BODY_BYTES}, "x"); + b.write("START"); + b.write("END", ${LARGE_BODY_BYTES - 3}); + return b.toString(); +})()`; + +const CLOSE_DELIMITED_READ = { + text: 'await s3.file("object.txt").text()', + bytes: 'new TextDecoder().decode(await s3.file("object.txt").bytes())', + "large text": 'await s3.file("object.txt").text()', +} as const; + +function closeDelimitedFixture(op: keyof typeof CLOSE_DELIMITED_READ) { + return ` +import net from "node:net"; + +const body = ${op === "large text" ? MAKE_LARGE_BODY : JSON.stringify(SMALL_BODY)}; +const server = net.createServer(socket => { + let buffer = Buffer.alloc(0); + socket.on("error", () => {}); + socket.on("data", chunk => { + buffer = Buffer.concat([buffer, chunk]); + if (buffer.indexOf("\\r\\n\\r\\n") === -1) return; + buffer = Buffer.alloc(0); + socket.write('HTTP/1.1 200 OK\\r\\nContent-Type: application/octet-stream\\r\\nETag: "etag"\\r\\nConnection: close\\r\\n\\r\\n'); + socket.write(body); + socket.end(); + }); +}); +await new Promise(resolve => server.listen(0, "127.0.0.1", resolve)); + +const s3 = new Bun.S3Client({ + accessKeyId: "key", + secretAccessKey: "secret", + bucket: "bucket", + endpoint: "http://127.0.0.1:" + server.address().port, +}); + +// Summarised rather than echoed: the large body would outrun the stdout pipe. +const received = ${CLOSE_DELIMITED_READ[op]}; +console.log(JSON.stringify({ length: received.length, head: received.slice(0, 5), tail: received.slice(-3) })); +server.close(); +`; +} + +function closeDelimitedExpectation(op: keyof typeof CLOSE_DELIMITED_READ) { + const body = op === "large text" ? largeBody() : SMALL_BODY; + return JSON.stringify({ length: body.length, head: body.slice(0, 5), tail: body.slice(-3) }); +} + +// The S3 client does not honor NO_PROXY, so an inherited proxy would hijack the +// request to the stub server. +const envWithoutProxy = { + ...bunEnv, + HTTP_PROXY: undefined, + HTTPS_PROXY: undefined, + http_proxy: undefined, + https_proxy: undefined, +}; + describe.each([true, false])("peer sends FIN: %p", sendFin => { test.concurrent.each(["write", "delete"] as const)( "S3Client.%s() resolves on a Content-Length: 0 + Connection: close response", @@ -44,15 +124,7 @@ describe.each([true, false])("peer sends FIN: %p", sendFin => { await using proc = Bun.spawn({ cmd: [bunExe(), "fixture.ts"], - // The S3 client does not honor NO_PROXY, so an inherited proxy would - // hijack the request to the stub server. - env: { - ...bunEnv, - HTTP_PROXY: undefined, - HTTPS_PROXY: undefined, - http_proxy: undefined, - https_proxy: undefined, - }, + env: envWithoutProxy, cwd: String(dir), stdout: "pipe", stderr: "pipe", @@ -66,3 +138,28 @@ describe.each([true, false])("peer sends FIN: %p", sendFin => { }, ); }); + +describe("close-delimited response body", () => { + test.concurrent.each(["text", "bytes", "large text"] as const)( + "S3Client %s reads a 200 with no Content-Length and no Transfer-Encoding", + async op => { + using dir = tempDir("s3-close-delimited", { "fixture.ts": closeDelimitedFixture(op) }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "fixture.ts"], + env: envWithoutProxy, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect({ stdout: stdout.trim(), exitCode }).toEqual({ stdout: closeDelimitedExpectation(op), exitCode: 0 }); + expect(stderr).not.toContain("S3Error"); + // Before the fix the completion callback carried no response metadata, so + // the task aborted the whole process and JS never saw an error at all. + expect(proc.signalCode).toBeNull(); + }, + ); +}); From d949f7a9aa5db07c78a5e60cbd73c68a8db8c538 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 6 Jul 2026 15:47:15 +0000 Subject: [PATCH 2/2] ci: retrigger