Skip to content

fix(build): rename AnyResponse::resume_() callers to resume() #36072

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jarred-Sumner merged 2 commits into from
Jul 27, 2026
Merged

fix(build): rename AnyResponse::resume_() callers to resume() #36072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
841b99a
fix(build): rename AnyResponse::resume_() callers to resume()
robobun Jul 27, 2026
bbcf82a
test: paused request body is released when handler responds without r…
robobun Jul 27, 2026
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
6 changes: 3 additions & 3 deletions src/runtime/server/RequestContext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2379,7 +2379,7 @@ where
if let Some(resp) = self.resp.take() {
if self.flags.request_body_paused() {
self.flags.set_request_body_paused(false);
resp.resume_();
resp.resume();
}
if self.flags.is_waiting_for_request_body() {
self.flags.set_is_waiting_for_request_body(false);
Expand Down Expand Up @@ -4123,7 +4123,7 @@ where
return;
}
if let Some(resp) = self.resp {
resp.resume_();
resp.resume();
}
}

Expand Down Expand Up @@ -4179,7 +4179,7 @@ where
}
}
if let Some(resp) = (*this).resp {
resp.resume_();
resp.resume();
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/runtime/webcore/streams.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1405,7 +1405,7 @@ impl<const SSL: bool, const HTTP3: bool> HTTPServerWritable<SSL, HTTP3> {
if let Some(res) = self.any_res() {
res.clear_on_writable();
// Release any request-body pause while `res` is live (see `end_already_responded_stream`).
res.resume_();
res.resume();
}
// `send_readable` drained the parked `try_end`, so uWS has
// `markDone()`d the response and dropped its `onAborted`.
Expand Down Expand Up @@ -1782,7 +1782,7 @@ impl<const SSL: bool, const HTTP3: bool> HTTPServerWritable<SSL, HTTP3> {

if let Some(res) = self.any_res() {
// Release any request-body pause while `res` is live (see `end_already_responded_stream`).
res.resume_();
res.resume();
}
// Both branches above fully ended the response through uWS, which
// `markDone()`s it and drops its `onAborted`.
Expand Down Expand Up @@ -1870,7 +1870,7 @@ impl<const SSL: bool, const HTTP3: bool> HTTPServerWritable<SSL, HTTP3> {
if let Some(res) = self.any_res() {
res.clear_on_writable();
// Release any request-body pause while `res` is live (see `end_already_responded_stream`).
res.resume_();
res.resume();
}
// `send_readable` drained the parked `try_end`/`end`, so uWS has
// `markDone()`d the response and dropped its `onAborted`.
Expand Down
48 changes: 48 additions & 0 deletions test/js/bun/http/serve.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3497,6 +3497,54 @@
}
});
}

it("releases a paused request body when the handler responds without reading it", async () => {
// The handler never touches req.body, so the pre-stream pause engages and is
// released by detach_response()'s resume once the response is sent. Without
// that resume the socket stays paused and the client never sees the response.
const TOTAL = 32 * 1024 * 1024;
const gate = Promise.withResolvers<void>();
const serverDone = Promise.withResolvers<void>();

using server = serve({
port: 0,
idleTimeout: 0,
maxRequestBodySize: TOTAL + 1,
error(e) {
serverDone.reject(e);
},
async fetch() {
await gate.promise;
serverDone.resolve();
return new Response("ignored");
},
});

const { sock, sentBeforeGate } = await pumpUploadUntilPlateau(server.port, TOTAL, 2);
try {
expect(sentBeforeGate).toBeGreaterThan(0);
expect(sentBeforeGate).toBeLessThan(TOTAL);

const response = new Promise<string>((resolve, reject) => {
let buf = "";
sock.removeAllListeners("data");
sock.on("data", d => {
buf += d.toString("latin1");
if (buf.includes("\r\n\r\n")) resolve(buf);
});
sock.once("error", reject);
sock.once("close", () => resolve(buf));
});

gate.resolve();
await serverDone.promise;
const resp = await response;
expect(resp).toStartWith("HTTP/1.1 200 ");
expect(resp).toContain("ignored");

Check warning on line 3543 in test/js/bun/http/serve.test.ts

View check run for this annotation

Claude / Claude Code Review

Test resolves on header terminator but asserts on body 

The response promise resolves on `buf.includes("\r\n\r\n")` (header terminator) but the test then asserts `resp.toContain("ignored")`, which is body content — if headers and body arrive in separate `data` events, the promise resolves with a headers-only snapshot and the body assertion fails spuriously. Resolve on `buf.includes("ignored")` instead (or drop the body assertion, since the `HTTP/1.1 200` line already proves the resume path fired).
Comment on lines +3528 to +3543

@claude claude Bot Jul 27, 2026

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.

🟡 The response promise resolves on buf.includes("\r\n\r\n") (header terminator) but the test then asserts resp.toContain("ignored"), which is body content — if headers and body arrive in separate data events, the promise resolves with a headers-only snapshot and the body assertion fails spuriously. Resolve on buf.includes("ignored") instead (or drop the body assertion, since the HTTP/1.1 200 line already proves the resume path fired).

Extended reasoning...

What the bug is

The data handler at line 3533 resolves the response promise as soon as buf.includes("\r\n\r\n") — i.e., once the HTTP header terminator has been seen. But line 3543 then asserts expect(resp).toContain("ignored"), which is the response body. The resolve condition does not cover everything that is asserted.

The code path that triggers it

sock.on("data", d => {
  buf += d.toString("latin1");
  if (buf.includes("\r\n\r\n")) resolve(buf);
});

JS strings are immutable: buf += d rebinds buf to a new string, and resolve(buf) captures the string value at that instant. If the ~100-byte response is delivered as two data events — first the status line + headers ending in \r\n\r\n, then the 7-byte ignored body — the first event satisfies buf.includes("\r\n\r\n") and resolves the promise with the headers-only snapshot. The second event appends to the closure-local buf, but the promise is already settled (the second resolve() is a no-op), and the close fallback at line 3536 is likewise a no-op on a settled promise. resp at line 3542 is then the headers-only string, and toContain("ignored") fails.

Why existing code doesn't prevent it

There is no framing that guarantees the body is present when the header terminator is seen. The close handler cannot rescue it because the promise has already resolved. Neighboring raw-socket tests in this file that assert on the body accumulate until close (e.g., toEndWith("\r\n\r\nhey") after the socket closes) rather than resolving on the header terminator.

Impact

REVIEW.md is explicit under Tests reviewers reject: "Buffer raw socket/stdout chunks to the protocol's framing before asserting." This is exactly the pattern that rule targets. In practice, uWS corks new Response("ignored") into a single ~100-byte write and loopback almost always delivers it in one segment, so the flake probability is very low — but it is non-zero across platforms/ASAN/debug scheduling, and it violates a rule that has blocked merges.

Step-by-step proof

  1. Server sends HTTP/1.1 200 OK\r\nContent-Type: text/plain;charset=utf-8\r\nContent-Length: 7\r\n\r\nignored in one res.end().
  2. Kernel delivers first data event containing everything up through ...\r\n\r\n (headers only). buf = headers; buf.includes("\r\n\r\n") → true → resolve(buf) with headers only.
  3. Kernel delivers second data event with "ignored". buf += "ignored" rebinds the local; resolve(buf) on a settled promise is a no-op.
  4. const resp = await response → headers-only string.
  5. expect(resp).toStartWith("HTTP/1.1 200 ") passes; expect(resp).toContain("ignored") fails.

Fix

Change the resolve condition to match what is asserted:

if (buf.includes("ignored")) resolve(buf);

Or drop line 3543 entirely — the HTTP/1.1 200 status line already proves detach_response()'s resume fired (without it the client would never see any response), so the body assertion adds no coverage.

} finally {
sock.destroy();
}
});
});

// https://github.com/oven-sh/bun/issues/32469
Expand Down
Loading