Skip to content

http: don't retry a fetch with a ReadableStream body on keep-alive disconnect#32798

Merged
Jarred-Sumner merged 4 commits into
mainfrom
farm/c0e7794d/fetch-no-retry-stream-body
Jun 27, 2026
Merged

http: don't retry a fetch with a ReadableStream body on keep-alive disconnect#32798
Jarred-Sumner merged 4 commits into
mainfrom
farm/c0e7794d/fetch-no-retry-stream-body

Conversation

@robobun

@robobun robobun commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

fetch() with an idempotent method (PUT) and a ReadableStream body, sent over a reused keep-alive connection that the peer resets mid-upload, aborts the whole Bun process:

panic: range start index 1031 out of range for slice of length 172

in send_initial_request_payload (src/http/lib.rs), at &temporary_send_buffer[self.state.request_sent_len..]. Any upstream, proxy, or load balancer that resets a reused connection mid-upload can kill a Bun client this way.

Repro (panics 4/4 on bun 1.4.0 and current main, exits 0 with this PR):

repro.ts
const RESP = "HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection: keep-alive\r\n\r\nok";
const srv = Bun.listen<{ buf: string; req: number; n: number }>({
  hostname: "127.0.0.1", port: 0,
  socket: {
    open(s) { s.data = { buf: "", req: 0, n: 0 }; },
    data(s, raw) {
      const d = s.data;
      if (d.req === 0) {                 // request 1: read fully, reply, keep alive
        d.buf += raw.toString("latin1");
        const i = d.buf.indexOf("\r\n\r\n"); if (i < 0) return;
        const cl = Number(d.buf.slice(0, i).match(/content-length: (\d+)/i)?.[1] ?? 0);
        if (d.buf.length < i + 4 + cl) return;
        d.req = 1; d.buf = ""; s.write(RESP); return;
      }
      d.n += raw.length;                 // request 2 (reused conn): RST mid-upload
      if (d.n > 8000) s.terminate();
    }, close() {}, error() {}, drain() {},
  },
});
const url = `http://127.0.0.1:${srv.port}/x`;
const CHUNK = new Uint8Array(1024);
const stream = () => { let n = 256; return new ReadableStream({ pull(c) { if (n-- <= 0) return c.close(); c.enqueue(CHUNK); } }); };
await fetch(url, { method: "PUT", body: new Uint8Array(64) }); // park a keep-alive connection
for (let i = 0; i < 20; i++) {
  await fetch(url, { method: "PUT", body: stream(), duplex: "half" }).catch(() => {});
}
console.log("SURVIVED");

Cause

  1. HTTPClient::on_close takes the idempotent retry regardless of the body type. A ReadableStream body is consumed as it is uploaded, so the retry replays a truncated body to begin with.
  2. The retry's start_() registers its still-connecting socket in the abort tracker (needed so an abort during a slow connect is honored). The JS side is still pushing chunks from the first attempt, so drain_queued_writes finds that socket and write_to_stream writes a body frame to it before on_open / the request headers. On loopback the TCP connect has already completed so the write succeeds and request_sent_len advances by the frame size. on_open then runs send_initial_request_payload, which slices the freshly rebuilt 172-byte header buffer at index 1031.

1031 is exactly one chunked frame of the 1024-byte chunk (400\r\n + 1024 + \r\n); switching the repro to 512-byte chunks moves the panic index to exactly 519, confirming the body frame lands before the headers.

Fix

  • on_close: only take the idempotent retry when the body is HTTPRequestBody::Bytes. Stream and Sendfile bodies are consumed as they are written and cannot be replayed; Go's net/http (Request.isReplayable) and curl apply the same rule. The request now fails with ECONNRESET like any other non-retryable request. The GET/bodyless retry behavior is unchanged (covered by the existing test/regression/issue/28706.test.ts).
  • write_to_stream: park until request_stage is Body or ProxyBody, mirroring the existing upgrade_state == Pending park in the same function. Body bytes must never reach the socket ahead of the request line, and request_sent_len still indexes the header buffer at that point. The buffered data is re-flushed by on_writable's Body arm once the headers are out.

How did you verify your code works?

New test in test/js/web/fetch/fetch-keepalive.test.ts (spawns a subprocess since the bug aborts the process). Without the fix it fails on the debug build with streamRequests: 8 instead of 4 (every attempt retried) and on a release build it additionally hits the panic. With the fix it passes 3/3. test/regression/issue/28706.test.ts and the stream-body / duplex / proxy / redirect / fetch-upgrade suites still pass.

…sconnect

An idempotent request (PUT) with a ReadableStream body sent over a
reused keep-alive connection that the peer resets mid-upload was
transparently retried by HTTPClient::on_close. A stream body is consumed
as it is uploaded, so the replay is silently truncated, and the chunks
still queued by the JS side get flushed onto the retry's not-yet-opened
socket ahead of the request headers. That advances request_sent_len past
the length of the rebuilt header buffer and send_initial_request_payload
panics, aborting the whole process:

    panic: range start index 1031 out of range for slice of length 172

Two changes:

- on_close: only take the idempotent retry when the body is
  HTTPRequestBody::Bytes. Stream and Sendfile bodies are consumed as
  they are written and cannot be replayed (Go's net/http and curl apply
  the same rule). The request now fails with ECONNRESET like any other
  non-retryable request.

- write_to_stream: park until request_stage is Body or ProxyBody.
  start_() registers the still-connecting socket in the abort tracker
  (needed for abort-during-connect), which let drain_queued_writes write
  body bytes to it before the headers went out. The buffered data is
  re-flushed by on_writable's Body arm once the headers are on the wire.
@robobun

robobun commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 8:22 PM PT - Jun 26th, 2026

@robobun, your commit 1865111 has 1 failures in Build #65236 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 32798

That installs a local version of the PR into your bun-32798 executable, so you can run:

bun-32798 --bun

@github-actions

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. Bun crash on http.zig sendInitialRequestPayload -> headerStr #23092 - Reports the same crash in sendInitialRequestPayload with an out-of-bounds index, which this PR fixes by preventing retry of stream bodies on keep-alive disconnect and guarding against body writes before headers are sent.

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #23092

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@robobun, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 4 minutes and 19 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ca60c434-c05e-41e1-9ebd-8bff9795d525

📥 Commits

Reviewing files that changed from the base of the PR and between c4d28c0 and 1865111.

📒 Files selected for processing (1)
  • src/http/lib.rs

Walkthrough

HTTPClient now only retries closed requests when the original body is reusable bytes, and write_to_stream now skips body writes until the request reaches body stages. The fetch keep-alive test adds a regression for a streaming PUT that disconnects mid-upload.

Changes

HTTP retry and write gating

Layer / File(s) Summary
Retry gate and regression test
src/http/lib.rs, test/js/web/fetch/fetch-keepalive.test.ts
on_close retries only HTTPRequestBody::Bytes(_), and the keep-alive test runs a subprocess/server flow that records repeated ECONNRESET failures for a streaming PUT after a warm keep-alive request.
Body write stage guard
src/http/lib.rs
write_to_stream now returns unless state.request_stage is Body or ProxyBody.

Possibly related PRs

  • oven-sh/bun#32462: Also changes src/http/lib.rs in the HTTPClient write path to stop work after socket closure.

Suggested reviewers

  • cirospaciari
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: preventing retries for ReadableStream fetches on keep-alive disconnects.
Description check ✅ Passed The description includes both required sections and provides a clear explanation plus verification details and repro information.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/web/fetch/fetch-keepalive.test.ts`:
- Around line 76-83: Shorten the regression note in the fetch keepalive test to
fit the 3-line comment limit by removing repeated explanation and collapsing the
retry/stream/panic behavior into one concise summary. Keep the key points tied
to the keep-alive retry case in the fetch keepalive test and preserve the
mention of the panic in send_initial_request_payload, but rewrite it as a
compact comment that still explains the bug scenario and subprocess abort.
- Around line 111-115: The keepalive test is terminating the socket too early in
the stream upload flow, so it may not exercise a consumed ReadableStream body or
the queued body-write path. Tighten the logic in fetch-keepalive.test.ts around
the stream upload check so the reset happens only after full headers and at
least one body byte have been received, then increment the counter and call
socket.terminate() from that guarded path. Use the existing socket.data.buffer
handling in the PUT /stream branch to ensure the test fails for the right
reason.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2d1095e5-a32c-4499-8d25-2764c1e13d46

📥 Commits

Reviewing files that changed from the base of the PR and between e384998 and f90dc48.

📒 Files selected for processing (2)
  • src/http/lib.rs
  • test/js/web/fetch/fetch-keepalive.test.ts

Comment thread test/js/web/fetch/fetch-keepalive.test.ts Outdated
Comment thread test/js/web/fetch/fetch-keepalive.test.ts
…ment

Review feedback: the server now waits for the full request headers plus at
least one body byte before resetting the connection, so the test provably
exercises a ReadableStream body that has started being consumed. Also
shortens the test's header comment to three lines.
@robobun

robobun commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator Author

Looked at the #23092 suggestion from the issue-finder bot and left it out of the description: it is a different crash.

#23092 panics inside header_str while build_request is still assembling the request (headerStr -> sendInitialRequestPayload -> onWritable -> firstCall off the TLS handshake callback), and the index is garbage (index 1864152300, len 1835), pointing at a corrupt header_entries StringPointer. #32702 targets that one and already carries Fixes #23092.

This PR's panic is two statements later in the same function, at &temporary_send_buffer[request_sent_len..], and the index is a meaningful byte count (exactly one chunked body frame, 1031 = 400\r\n + 1024 + \r\n). The cause is the stale/early request_sent_len on the retry path, not a bad header pointer.

Both CodeRabbit comments are addressed in d36fad9: the test server now waits for the full request headers plus at least one body byte before resetting the connection, so the ReadableStream body has provably started being consumed, and the test's header comment is trimmed to three lines. Re-verified the fail/pass split after the change: the unfixed release binary still hits the panic 3/3, and the debug build without the src/ change still fails with streamRequests: 8 instead of 4.

darwin-aarch64 test-bun failed on a buildkite artifact download timeout
before running any test; no other job in build 65218 failed.
Comment thread src/http/lib.rs Outdated
@robobun

robobun commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator Author

CI status

The diff is done and the gate proof holds (fail-before on the unfixed binary, pass-after with the fix, 3/3 reruns each way). CI is red only on lanes my change cannot touch, and the same lanes are red on unrelated PRs built at the same time. I have already used one retrigger, so I am not pushing more; a maintainer can retry the three job groups from Buildkite or re-run the build.

Build: https://buildkite.com/bun/bun/builds/65236 (head 18651111ef)

  1. darwin 26 aarch64 - test-bun (x2): buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'. Zero tests ran. The same failure hit the previous build of this PR (https://buildkite.com/bun/bun/builds/65218) and an unrelated PR built at the same time (https://buildkite.com/bun/bun/builds/65239). Infra.

  2. windows 2019 x64, windows 2019 x64-baseline, windows 11 aarch64 - test-bun: every one fails on the same single file, test/js/bun/sourcemap/internal-sourcemap-roundtrip.test.ts, asserting sources: ["../in.js"] but receiving ["..\in.js"] (Windows path separator). That test was added by Don't panic generating a sourcemap for a source ending in a truncated UTF-8 sequence #32774 and is in the sourcemap emitter; this PR only touches the fetch HTTP client (HTTPClient::on_close and write_to_stream in src/http/lib.rs). The same three Windows lanes fail on the same file in an unrelated PR's build from the same window (https://buildkite.com/bun/bun/builds/65237). None of these shards ran test/js/web/fetch/fetch-keepalive.test.ts, and no failure anywhere in the build involves the HTTP client.

The only other noise is the flaky annotation (s3, node-http-connect, solc), all of which passed on retry.

@Jarred-Sumner Jarred-Sumner merged commit a2290aa into main Jun 27, 2026
71 of 76 checks passed
@Jarred-Sumner Jarred-Sumner deleted the farm/c0e7794d/fetch-no-retry-stream-body branch June 27, 2026 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants