Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/http/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1899,6 +1899,10 @@

if self.allow_retry
&& self.method.is_idempotent()
// Only a Bytes body can be rebuilt from `original_request_body`.
// Stream/Sendfile bodies are consumed as they are written, so a
// retry would silently replay a truncated request.
&& matches!(self.state.original_request_body, HTTPRequestBody::Bytes(_))
&& self.state.response_stage != ResponseStage::Body
&& self.state.response_stage != ResponseStage::BodyChunk
{
Expand Down Expand Up @@ -2882,6 +2886,16 @@

pub fn write_to_stream<const IS_SSL: bool>(&mut self, socket: HttpSocket<IS_SSL>, data: &[u8]) {
bun_core::scoped_log!(fetch, "flushStream");
// Never write body bytes before the request headers: drain_queued_writes
// can reach this via the not-yet-opened socket start_() registers in the
// abort tracker, and request_sent_len still indexes the header buffer.
// The data stays buffered; on_writable's Body/ProxyBody arm re-flushes.

Check warning on line 2892 in src/http/lib.rs

View check run for this annotation

Claude / Claude Code Review

Comment exceeds 3-line limit (CLAUDE.md rule #13)

Nit: this comment is 4 lines, but CLAUDE.md rule #13 caps code comments at 3 — the same rule was just applied to the test header in d36fad9, and the other new comment at lines 1902-1904 is 3 lines. Could fold the last sentence into line 3 (or drop it, since `on_writable`'s re-flush is implied by "the data stays buffered").
Comment thread
robobun marked this conversation as resolved.
Outdated
if !matches!(
self.state.request_stage,
RequestStage::Body | RequestStage::ProxyBody
) {
return;
}
// reshaped for borrowck — copy out the Copy bits we need
// (`upgrade_state`, the stream-buffer NonNull, `ended`) so the
// `&mut self.state.original_request_body` borrow is dropped before any
Expand Down
99 changes: 98 additions & 1 deletion test/js/web/fetch/fetch-keepalive.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect, test } from "bun:test";
import { tls } from "harness";
import { bunEnv, bunExe, tls } from "harness";

test("keepalive", async () => {
using server = Bun.serve({
Expand Down Expand Up @@ -72,3 +72,100 @@ test("fetch does not reuse a pooled TLS connection for a request with a differen
const plain = await get();
expect(plain).not.toBe(overrideA);
});

// A reused keep-alive connection reset during a streaming PUT must reject with
// ECONNRESET, not retry: the stream body is already consumed, and the retry
// panicked in send_initial_request_payload. Subprocess: the panic aborts the process.
test("PUT with a ReadableStream body is not retried on keep-alive disconnect", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const CRLF = String.fromCharCode(13, 10);
let warmRequests = 0;
let streamRequests = 0;

const server = Bun.listen({
hostname: "127.0.0.1",
port: 0,
socket: {
open(socket) { socket.data = { buffer: "" }; },
data(socket, data) {
socket.data.buffer += data.toString("latin1");
if (!socket.data.buffer.includes(CRLF)) return;
if (socket.data.buffer.startsWith("PUT /warm")) {
// Wait for the full 4-byte body before replying keep-alive.
const i = socket.data.buffer.indexOf(CRLF + CRLF);
if (i < 0 || socket.data.buffer.length < i + 4 + 4) return;
warmRequests++;
socket.data.buffer = "";
socket.write("HTTP/1.1 200 OK" + CRLF + "Content-Length: 2" + CRLF + "Connection: keep-alive" + CRLF + CRLF + "ok");
return;
}
if (socket.data.buffer.startsWith("PUT /stream")) {
// Wait for the full headers plus at least one body byte so the
// stream body has actually started being consumed before the reset.
const i = socket.data.buffer.indexOf(CRLF + CRLF);
if (i < 0 || socket.data.buffer.length <= i + 4) return;
streamRequests++;
socket.data.buffer = "";
// Reset the connection mid-upload.
socket.terminate();
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
},
close() {},
error() {},
drain() {},
},
});

const base = "http://127.0.0.1:" + server.port;
const chunk = new Uint8Array(1024);
const streamBody = () => {
let pending = 32;
return new ReadableStream({
pull(c) {
if (pending-- <= 0) return c.close();
c.enqueue(chunk);
},
});
};

const errors = [];
for (let i = 0; i < 4; i++) {
// Park a keep-alive connection so the stream PUT reuses it.
await (await fetch(base + "/warm", { method: "PUT", body: "warm" })).text();
try {
await fetch(base + "/stream", { method: "PUT", body: streamBody(), duplex: "half" });
errors.push(null);
} catch (e) {
errors.push(e && (e.code || e.name));
}
}

server.stop();
console.log(JSON.stringify({ warmRequests, streamRequests, errors }));
process.exit(0);
`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

// If the subprocess crashed there is no JSON; surface the raw output instead.
const result = stdout.startsWith("{") ? JSON.parse(stdout.trim()) : { stdout, stderr };
expect({ result, exitCode }).toEqual({
// Without the fix every attempt is retried on a fresh connection, so the
// server sees each PUT /stream twice (streamRequests === 8).
result: {
warmRequests: 4,
streamRequests: 4,
errors: ["ECONNRESET", "ECONNRESET", "ECONNRESET", "ECONNRESET"],
},
exitCode: 0,
});
});
Loading