Bun.serve: apply TCP backpressure to a request body the handler reads slowly - #36006
Code review found 2 important issues
Found 5 candidates, confirmed 5. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 2 |
| 🟡 Nit | 3 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/runtime/server/RequestContext.rs:2378-2381 |
Paused request-body socket is never resumed on the end_already_responded_stream path (keep-alive hang) |
| 🔴 Important | src/runtime/server/RequestContext.rs:3968-3971 |
Request body deadlocks when consumed via ByteStream buffer_action fastpath after req.body access |
| 🟡 Nit | src/runtime/server/RequestContext.rs:4094-4104 |
pause_request_body_socket lacks the Windows opt-out that the sibling node:http path carries |
| 🟡 Nit | src/runtime/server/RequestContext.rs:3960-3971 |
signal_drained re-entrantly creates a second &mut RequestContext while on_buffered_body_chunk holds one |
| 🟡 Nit | test/js/bun/http/serve.test.ts:3353-3356 |
New backpressure tests: failure paths not wired to reject awaited promises; socket not released on failure |
Annotations
Check failure on line 2381 in src/runtime/server/RequestContext.rs
claude / Claude Code Review
Paused request-body socket is never resumed on the end_already_responded_stream path (keep-alive hang)
`end_already_responded_stream()` is the one HTTP/1 teardown path that bypasses `detach_response()` — it `self.resp.take()`s and manually mirrors the old flag-clearing, but the new `REQUEST_BODY_PAUSED` → `resp.resume_()` block was not mirrored here (nor does uWS `markDone()` touch the uSocket's `is_paused` bit). A keep-alive client that PUTs a >1 MiB body while the handler ignores `req.body` and returns `new Response(aReadableStream)` ends up on this path with the socket still paused: READABLE s
Check failure on line 3971 in src/runtime/server/RequestContext.rs
claude / Claude Code Review
Request body deadlocks when consumed via ByteStream buffer_action fastpath after req.body access
This deadlocks any request body > ~1 MiB when the handler touches `req.body` before calling `.text()`/`.arrayBuffer()`/`.json()` (or uses `Bun.readableStreamToText(req.body)`). That path goes through `ByteStream::to_buffered_value`'s `buffer_action` fastpath — not `on_start_buffering` — so `REQUEST_BODY_BUFFER_ALL` is never set, the HWM check pauses the socket after ~1 MiB, and nothing ever resumes it. Gate the pause on `bytes.buffer_action.get().is_none()` (same intent as the buffer-all flag),
Check warning on line 4104 in src/runtime/server/RequestContext.rs
claude / Claude Code Review
pause_request_body_socket lacks the Windows opt-out that the sibling node:http path carries
The only sibling request-body `pause()` site, `NodeHTTPResponse::do_pause` (NodeHTTPResponse.rs:1367-1371), still wraps its call in `#[cfg(not(windows))]` with a "TODO: figure out why windows is not emitting EOF with UV_DISCONNECT", while `pause_request_body_socket` pauses unconditionally. The libuv backend's paused-socket probe (libuv.c:95-132, `fin_deferred` sweep, unconditional `UV_DISCONNECT` arming) appears to have made that guard stale — but per REVIEW.md's sibling-site rule the two paths
Check warning on line 3971 in src/runtime/server/RequestContext.rs
claude / Claude Code Review
signal_drained re-entrantly creates a second &mut RequestContext while on_buffered_body_chunk holds one
`on_buffered_body_chunk` holds `let this = unsafe { &mut *this }` across `bytes.on_data(...)`, and `on_data` now synchronously calls `signal_drained()` → `on_request_body_stream_drained_callback`, which does `bun_ptr::callback_ctx::<Self>(ctx)` — a second live `&mut RequestContext` aliasing the outer one. This violates `callback_ctx`'s documented uniqueness contract (bun_core/lib.rs:689-693) and is Miri-UB under Stacked Borrows, though the function-pointer indirection means it won't miscompile t
Check warning on line 3356 in test/js/bun/http/serve.test.ts
claude / Claude Code Review
New backpressure tests: failure paths not wired to reject awaited promises; socket not released on failure
The three new backpressure tests only ever `resolve` `serverDone` / `gotFirstChunk` — if the fetch handler throws (e.g. `reader.read()` or `req.arrayBuffer()` rejects on a regression in the very code under test), the awaited promise never settles and the test hangs to the suite timeout with no diagnostic. `sock.destroy()` is also placed inline after the awaits (and after `expect(sent).toBeLessThan(TOTAL)` in the third test) rather than in `try/finally`, so a hang or assertion failure leaks the r