fetch: make the response-header phase an absolute deadline - #36146
fetch: make the response-header phase an absolute deadline#36146robobun wants to merge 1 commit into
Conversation
The HTTP/1.1 client's only response-phase bound is the socket idle timer, armed at on_open and re-armed on every read. handle_on_data_headers' short-read path re-armed it on each partial header chunk, so a server that drips one header line per <idle-timeout> kept the request (and its socket and request-cap slot) pinned indefinitely. undici rejects the identical drip with HeadersTimeoutError at its absolute 300s headers deadline. Drop the re-arm from the short-read path so the timer armed at request- write stays monotonic for the whole header phase, and re-arm once after handle_response_metadata succeeds so the body phase starts with a fresh idle window. Body reads continue to re-arm per byte as before. The test runs a child with BUN_CONFIG_HTTP_IDLE_TIMEOUT=5 (two 4s sweep ticks; 1-4s map to a single tick and would fire on the next sweep regardless of re-arming, masking the bug) against a raw TCP server that drips headers every second. A second request whose body drips for 12s guards the body-idle semantics.
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Checked the two bot-suggested issues: #26066 is a 1.3.x regression under sustained AWS SDK load and #19590 is a gzip body freeze against a Google endpoint. Neither reporter describes a server that drips response headers (AWS and Google don't), so I'm not adding |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #36145, which opened first with the same |
There was a problem hiding this comment.
Beyond the inline finding, I also checked the 100-Continue early-return path in handle_on_data_headers (lib.rs:3714-3720) — it now leaves the timer untouched too, which is correct since the absolute header deadline should cover the wait for the final status. The new header→body re-arm at line 3740 is in the shared function, so it applies to both direct and tunneled requests once the proxy on_data re-arm is gated.
Extended reasoning...
Verified the proxy-tunnel finding against src/http/lib.rs:3831-3835 and src/http/ProxyTunnel.rs:340-352 — on_data unconditionally re-arms before dispatching to handle_on_data_headers when proxy_tunnel.is_some(), so the short_read!() change is bypassed on that path. Separately confirmed the 1XX-only early return no longer re-arms (correct for an absolute header deadline) and that the new post-handle_response_metadata re-arm is on the shared code path so body-idle semantics resume for both entry points.
| // Deliberately does NOT re-arm the socket timer: the timer armed at | ||
| // request-write time stays monotonic for the whole response-header | ||
| // phase, so it acts as an absolute headers deadline (undici's | ||
| // `headersTimeout`). Re-arming here let a server that drips one | ||
| // header line per <idle-timeout> pin the request forever. The timer | ||
| // is re-armed below once headers are complete, for the body phase. |
There was a problem hiding this comment.
🔴 The proxy-tunnel path still re-arms the idle timer on every dripped header byte: on_data at src/http/lib.rs:3833 calls self.set_timeout(&socket) unconditionally before dispatching to receive() → handle_on_data_headers, so the removal of set_timeout from short_read!() is moot when the origin is behind an HTTPS CONNECT proxy. The non-tunnel arm (3847-3849) already skips the re-arm for Pending|Headers; gating the re-arm at 3833 on self.state.response_stage != HTTPStage::ProxyHeaders (mirroring that structure) closes the sibling site — otherwise the same drip attack still pins the request forever via a proxy, and the new comment claiming the timer "stays monotonic for the whole response-header phase" is false on that path.
Extended reasoning...
What the bug is
The PR converts the response-header phase from an idle timer into an absolute deadline by removing self.set_timeout(&socket) from the short_read!() macro in handle_on_data_headers, so partial header reads no longer re-arm the socket timer. The new comment (src/http/lib.rs:3627-3632) documents the invariant: "the timer armed at request-write time stays monotonic for the whole response-header phase".
However, handle_on_data_headers has two entry points, and the fix only covers one. For a request routed through an HTTPS CONNECT proxy, encrypted TLS records from the origin arrive at HTTPClient::on_data (lib.rs:3819), which at lines 3831-3835 does:
if self.proxy_tunnel.is_some() {
// if we have a tunnel we dont care about the other stages, we will just tunnel the data
self.set_timeout(&socket); // <-- unconditionally re-arms on EVERY chunk
self.proxy_tunnel_mut().unwrap().receive(incoming_data);
return;
}receive() decrypts and, when response_stage == ProxyHeaders, dispatches to handle_on_data_headers (src/http/ProxyTunnel.rs:340-352). So the outer on_data has already re-armed the idle timer before short_read!() ever runs — the removal of set_timeout from the macro has no effect on this path.
Why the non-tunnel path is different
Contrast the direct (non-tunnel) branch of the same function at lines 3847-3893: the match self.state.response_stage sends Pending | Headers straight to handle_on_data_headers without re-arming (line 3849); only the Body (3853) and BodyChunk (3871) arms call set_timeout. The tunnel branch at 3833 re-arms unconditionally regardless of response stage, so the two entry points are now asymmetric.
Step-by-step proof
Consider an origin behind an HTTPS CONNECT proxy, with BUN_CONFIG_HTTP_IDLE_TIMEOUT=5, that drips one header line per second (the exact scenario in the new test, but through a proxy):
- Request is written; idle timer armed for 5s.
- Origin sends
HTTP/1.1 200 OK\r\ninside a TLS record → arrives aton_data(lib.rs:3819). proxy_tunnel.is_some()→ line 3833 callsself.set_timeout(&socket)→ timer re-armed to 5s.receive()decrypts →response_stage == ProxyHeaders→handle_on_data_headers(ProxyTunnel.rs:345/349).- picohttpparser returns
Status::Partial→short_read!()persists the tail and returns (no longer re-arms — but step 3 already did). - 1s later, origin sends
X-Drip-0: v\r\n→ back to step 2. Timer re-armed again. - Since 1s < 5s, the sweep never fires. Loop forever.
The direct-connection variant of this exact sequence is what the PR fixes and tests; the tunneled variant is untouched.
Impact
An origin (or misbehaving upstream) reachable via an HTTPS proxy can drip one header line per <idle-timeout> and pin the request, its socket, and a request-cap slot indefinitely — precisely the resource-pinning hole described in the PR body, on the sibling variant. The added comment now misdocuments the invariant for tunneled requests. Per REVIEW.md — "Fix the whole class in the same PR — grep for every sibling site sharing the pattern… SSL/non-SSL variants… If a site is intentionally excluded, say so in the PR" — this is required scope, and the PR description does not mention excluding the proxy path.
Suggested fix
Gate the re-arm at line 3833 on the response stage, mirroring the non-tunnel match:
if self.proxy_tunnel.is_some() {
if self.state.response_stage != HTTPStage::ProxyHeaders {
self.set_timeout(&socket);
}
self.proxy_tunnel_mut().unwrap().receive(incoming_data);
return;
}(ProxyHandshake could be excluded too for symmetry, though the handshake completes before header parsing so it's less material.) The new set_timeout after handle_response_metadata (line 3737) already covers the header→body boundary for both paths, so body-phase re-arm is preserved.
Gives the body phase a fresh idle window instead of whatever was left of the header-phase deadline, matching undici where headersTimeout and bodyTimeout are independent. Folded from #36146.
…er block (#36145) A server that trickles one response-header byte at a time, each interval shorter than the request's idle timeout, can keep a `fetch()` alive indefinitely. The HTTP client re-arms its socket idle timer inside the `short_read!` path of `handle_on_data_headers`, so every dripped byte resets the clock. A fully silent stall in the same phase is already bounded by the same timer (armed at `on_open`); only the drip defeats it. ## Reproduction ```js import net from "net"; const FULL = "HTTP/1.1 200 OK\r\nContent-Length: 2\r\n\r\nok"; const server = net.createServer(sock => { sock.on("data", () => {}); let i = 0; const iv = setInterval(() => { if (sock.destroyed) return clearInterval(iv); if (i < 10) sock.write(FULL[i++]); else { clearInterval(iv); sock.end(FULL.slice(i)); } }, 2000); }); await new Promise(r => server.listen(0, "127.0.0.1", r)); await fetch(`http://127.0.0.1:${server.address().port}/`, { timeout: 5000 }); // 1.4.0-canary: resolves 200 after ~22s. With this change: TimeoutError at ~5-9s. ``` The same shape with no bytes written (silent stall) already rejects with `TimeoutError: The operation timed out.` on the existing build, so `AbortSignal.timeout()` is the only way to bound the drip case today. ## Change - Drop the `set_timeout` call from `short_read!` in `handle_on_data_headers`. The timer stays as armed by `on_open` / `on_writable`, so it is an absolute deadline for the header block (undici `headersTimeout` semantics). - Gate the proxy-tunnel `on_data` re-arm on `response_stage == Body | BodyChunk`, mirroring the non-proxy dispatch, so the same deadline holds for HTTPS through a CONNECT proxy. - Re-arm once right after `handle_response_metadata` succeeds so the body phase starts with a fresh idle window rather than whatever was left of the header deadline (folded from #36146). Body reads continue to re-arm per chunk (undici `bodyTimeout` semantics). - Update the `IDLE_TIMEOUT_SECONDS` doc comment to describe the new header-phase behaviour. The default deadline is unchanged (300 s / `BUN_CONFIG_HTTP_IDLE_TIMEOUT` / per-request `timeout`), and `{timeout: false}` still disables it. ## Verification New test in `test/js/web/fetch/fetch.test.ts`: a raw `net.Server` drips 10 header bytes at 2 s each against `{timeout: 5000}` and must reject with `TimeoutError`, then drips a 5-byte body after a burst header block and must resolve with 200. ``` $ USE_SYSTEM_BUN=1 bun test test/js/web/fetch/fetch.test.ts -t "absolute deadline" (fail) header drip resolves {status: 200, body: "hello"} after 22011 ms $ bun bd test test/js/web/fetch/fetch.test.ts -t "absolute deadline" (pass) [18250 ms] ``` `bun-install-stalled-tls.test.ts`, `fetch-keepalive.test.ts`, the adjacent "explicit numeric \`timeout\` extends the socket idle deadline" test, and `proxy.test.ts` / `proxy-stress-lifecycle.test.ts` / `proxy-stress-matrix.test.ts` (496 proxy tests total, including the trickled-tunnel-bytes cases) all pass on the debug build. ## Scope HTTP/1 only. Related: #33338 adds `connectTimeout` / `socketTimeout` / whole-request `timeout` as per-request options with no change to the header-phase re-arm; this change is independent and composes with it. A `headersTimeout` option distinct from the body-phase idle value can follow once the per-request plumbing in #33338 lands. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/fetch.test.ts <!-- robobun:evidence:end -->
The HTTP/1.1 client's only response-phase bound is the socket idle timer (default 300s,
BUN_CONFIG_HTTP_IDLE_TIMEOUT), armed aton_openand re-armed on every read.handle_on_data_headers' short-read path re-armed it on each partial header chunk, so a server that drips one header line per<idle-timeout>pins the request, its socket, and a request-cap slot indefinitely. Every other stalled-response state (silent TLS handshake, zero response bytes, mid-headers silence, mid-body stall) times out; the drip is the one state that does not. undici rejects the identical drip withHeadersTimeoutErrorat its absolute 300s headers deadline.Repro
The only way to bound this today is
AbortSignal.timeout(N).Cause
src/http/lib.rshandle_on_data_headersshort_read!()calledself.set_timeout(&socket)on every partial parse, so any header byte reset the idle window.Fix
Drop the re-arm from
short_read!()so the timer armed at request-write stays monotonic for the whole response-header phase, turning it into an absolute headers deadline (undici parity). Re-arm once afterhandle_response_metadatasucceeds so the body phase starts with a fresh idle window; body reads continue to re-arm per byte as before.Verification
test/js/web/fetch/fetch.test.tsgains a test that spawns a child withBUN_CONFIG_HTTP_IDLE_TIMEOUT=5against a raw TCP server./dripdrips one header line per second and never finishes the header block;/bodysends complete headers immediately then drips one body byte per second for 12s./dripmust reject withTimeoutErrorinside the watchdog window (it stayed"pending"before);/bodymust still resolve with the full payload, proving body-idle re-arm is preserved.The idle window is 5s because that is the smallest value that maps to two uSockets sweep ticks (
(seconds+3)>>2). 1-4s map to a single tick and fire on the next 4s sweep regardless of re-arming, which would mask the bug.The neighbouring
{timeout: N}idle-override test andfetch-keepalive.test.tspass unchanged.Related to #2994 (connect-phase timeout) and complementary to #33338, which adds per-request timeout option names but keeps the re-arm on every read, so a header drip would still pin
socketTimeout.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/fetch.test.ts