fetch: make the idle timer an absolute deadline for the response header block - #36145
Conversation
…er block The HTTP client re-armed its socket idle timer on every partial header read (the short_read! path in handle_on_data_headers), so a server that trickled one header byte per interval shorter than the configured idle timeout could keep a request alive indefinitely. A silent stall in the same phase was already bounded; only the drip defeated it. Stop re-arming on a partial header read. The timer stays as armed by on_open / on_writable (the last outbound write), which turns it into an absolute deadline for the header block to complete, matching undici's headersTimeout semantics. The body path continues to re-arm per chunk (undici bodyTimeout semantics), so a slow-but-steady body is still accepted.
|
Reproduced with the |
WalkthroughChangesHTTP idle-timeout behavior
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Re #26066: I don't think this PR fixes it. That report describes requests to AWS that go silent after ~400-500 requests ("request is sent but no response is ever received"). A fully silent stall was already bounded by the 300 s idle timer before this change; the reporters are tripping a 30 s manual timeout first, so they never observe it, but it is there. This PR only changes the case where the server does send bytes during the header phase, just too slowly to ever complete, and there's no evidence AWS endpoints do that. #26066 reads more like a keep-alive pool / half-open socket problem. Leaving the description as-is. |
|
Opened #36146 for the same bug a few minutes after this one; closing that in favor of this PR. One difference worth considering: #36146 also re-arms the timer once right after |
The outer on_data re-armed the idle timer on every tunnelled byte before decryption, so the header-phase absolute deadline did not hold for HTTPS-through-CONNECT-proxy requests. Gate the re-arm on response_stage == Body/BodyChunk, mirroring the non-proxy dispatch. Also update the IDLE_TIMEOUT_SECONDS doc comment to match the new header-phase semantics.
|
Updated 4:03 PM PT - Jul 27th, 2026
✅ @robobun, your commit 97e21033b3ac22682523ed9de9a3d3a469b8d736 passed in 🧪 To try this PR locally: bunx bun-pr 36145That installs a local version of the PR into your bun-36145 --bun |
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.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/http/lib.rs:3646-3649— The idle timer is not re-armed at the headers→body boundary: afterhandle_response_metadatasucceeds, both theto_read.is_empty()return (lib.rs:3770-3776) and theBodyarm (3778-3790) proceed without aset_timeout, so a body that stalls right after headers-complete inherits only the remainder of the header-phase window instead of a freshidle_timeout_seconds— undici startsbodyTimeoutfresh at headers-complete, and the siblingBodyChunkarm at 3792 already re-arms, so this is internally inconsistent. This regresses the multi-chunk-header case (previouslyshort_read!left the timer armed near headers-complete) and was raised in this thread via #36146 but not folded in; the fix is to callself.set_timeout(&socket)once right afterclone_metadata.Extended reasoning...
What the bug is
This PR removes the
set_timeoutcall fromshort_read!so the idle timer becomes an absolute deadline for the response header block. That is correct for the header phase, but the timer is never re-armed at the point where the header phase ends and the body phase begins. The PR's new comment (and the updated doc onIDLE_TIMEOUT_SECONDS) claims "undiciheadersTimeoutsemantics" with the body path re-arming per chunk — but undici also starts a freshbodyTimeoutwindow when headers complete, and this change does not.Code path
In
handle_on_data_headers, oncehandle_response_metadatasucceeds andresponse_stagetransitions toBody/BodyChunk:to_read.is_empty()(lib.rs:3770-3776): returns afterclone_metadatawith noset_timeout. If the server then goes silent, the request times out with only the remainder of the window that was armed aton_writable, not a freshidle_timeout_seconds.Bodyarm (3778-3790): callshandle_response_body(to_read, true)with no precedingset_timeout(andhandle_response_bodydoes not re-arm internally).BodyChunkarm (3791-3792): does callself.set_timeout(&socket)beforehandle_response_body_chunked_encoding.
The
BodyandBodyChunkarms are siblings handling the same transition, and only one of them re-arms — that internal inconsistency alone signals this is unintentional. Subsequent body bytes do re-arm via theon_dataBody/BodyChunkarms, so this only bites when the body stalls immediately after headers-complete.Why this is a regression
Before this PR,
short_read!re-armed on every partial header read. So when headers arrived over multipleon_datacalls, the timer was left armed at the last partial-header read — i.e. very close to headers-complete. A subsequent silent body stall then had roughly a fullidle_timeout_secondsbefore firing. After this PR, the timer stays as armed aton_writable(t≈0), so the body's first-stall window shrinks toidle_timeout_seconds − header_arrival_time.Step-by-step proof
Take
{timeout: 5000}against a server that drips the header block one byte at a time over 4 s (chunks at t=0.5, 1.0, …, 4.0 s; the t=4.0 s chunk completes the headers with no trailing body bytes), then stalls before sending the first body byte until t=8 s.- Before this PR: each partial-header
on_dataat t=0.5…3.5 s callsshort_read!, which re-arms the timer. At t=3.5 s the timer is freshly armed. The t=4.0 s chunk completes the headers;to_read.is_empty()returns without re-arming, but the timer is still armed from t=3.5 s. It would fire at ~t=8.5 s (plus the 4 s tick slop), so the body byte at t=8 s arrives in time and the request succeeds. - After this PR:
short_read!no longer re-arms. The timer stays as armed byon_writableat t≈0 s and fires at ~t=5-9 s. The body byte at t=8 s is likely past the deadline; the request fails withTimeoutErrorwhere it previously succeeded.
For
Content-Lengthbodies this always applies (theBodyarm never re-arms at the transition); for chunked bodies it applies only when the header-completing packet carried no trailing bytes (theBodyChunkarm at 3792 re-arms when it does).Impact and prior notice
This was explicitly raised in the PR thread by robobun on 2026-07-27 ("#36146 also re-arms the timer once right after
handle_response_metadatasucceeds … undici'sbodyTimeoutstarts fresh at headers-complete"). The follow-up commit b278169 addressed the proxy-tunnel gate and theIDLE_TIMEOUT_SECONDSdoc comment but did not fold this in. Per REVIEW.md "Fix the whole class in the same PR", the header-phase and body-phase timer semantics are one concern, and the updated doc comment now over-claims ("body-phase reads re-arm") for the first body window.Suggested fix
Call
self.set_timeout(&socket)once immediately afterself.clone_metadata(&response)(i.e. right afterhandle_response_metadatareturnsContinueand the stage has transitioned toBody/BodyChunk). That gives the body a freshidle_timeout_secondswindow at headers-complete, matches undici'sbodyTimeoutstart point, and makes theBodyandBodyChunkarms consistent (the explicitset_timeoutat 3792 can then be dropped).
The headers-complete re-arm in 94b065e made the BodyChunk-arm set_timeout in handle_on_data_headers redundant (same value, same on_data call). Drip test: run /h and /b concurrently and shrink the body to 3 bytes so wall-clock drops from ~18s to ~8s.
|
Re the headers→body boundary re-arm: that was folded in at 94b065e (one |
… on_data maybe_pause_receive early-returns when proxy_tunnel.is_some(), so receive_paused is never true inside the proxy on_data branch. Document the invariant with a debug_assert instead of a conjunct that can never be false.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/http/lib.rs`:
- Around line 3822-3830: Add a test covering the proxy_tunnel entry point with
HTTPS sent through a CONNECT proxy, using both header-drip and body-drip
behavior to exercise the ResponseStage::Body | BodyChunk timeout gate. Keep the
existing direct HTTP coverage and ensure the new case validates timeout behavior
for the CONNECT tunnel path.
In `@test/js/web/fetch/fetch.test.ts`:
- Around line 3092-3098: Update the sock.once("data") request-handling logic to
accumulate incoming bytes until the HTTP header terminator is received, rather
than determining headerDrip from the first chunk. Parse the framed request
target with the repository’s real HTTP parser, then select the /h or /b fixture
mode from that parsed target while preserving the existing response-drip
behavior.
- Line 3117: Update the server setup promise around server.listen in the fetch
test so it rejects when the server emits an error, while still resolving from
the successful listen callback. Wire the server's failure event to the promise
rejection before starting the listener, preserving the existing host and port
behavior.
🪄 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: 0ad52fb7-827b-4ccd-967b-9ebcbbe626d3
📒 Files selected for processing (2)
src/http/lib.rstest/js/web/fetch/fetch.test.ts
There was a problem hiding this comment.
No bugs found this pass. This is a small but user-visible behavioral change to fetch() timeout semantics on the HTTP/1 client hot path (header-phase reads no longer re-arm the idle timer), so it's worth a human look before landing.
Checked this round: the headers-complete re-arm at lib.rs:3731 correctly covers both direct and CONNECT-tunnel dispatch (ProxyTunnel.rs:345/349 pass the real outer socket); the 100-Continue early return at :3715 was never a short_read! path so no regression there; the proxy-tunnel debug_assert!(!receive_paused) holds because maybe_pause_receive bails on proxy_tunnel.is_some(); the removed BodyChunk-arm re-arm is fully subsumed by :3731.
Extended reasoning...
Overview
Removes set_timeout from the short_read! macro in handle_on_data_headers so partial response-header reads no longer reset the socket idle timer. Adds a single re-arm at headers-complete (right after handle_response_metadata returns Ok) so the body phase starts with a fresh window, drops the now-redundant re-arm in the piggybacked BodyChunk arm, and gates the proxy-tunnel on_data re-arm on response_stage == Body | BodyChunk (with a debug_assert! documenting that receive_paused cannot be set on the tunnel path). Updates the IDLE_TIMEOUT_SECONDS doc comment. New ~8s test in fetch.test.ts proves a header drip now hits TimeoutError while a body drip still resolves 200.
Security risks
None introduced. If anything this is a client-side hardening: a hostile or misbehaving server can no longer hold a Bun fetch() open indefinitely by trickling one header byte per interval. No new input parsing, no allocation sizing on untrusted data, no auth/crypto surface.
Level of scrutiny
Medium-high. src/http/lib.rs on_data / handle_on_data_headers is the HTTP/1 client hot path used by every fetch(), bun install, etc. The diff is small and mechanically simple, but it changes user-visible timing semantics: a server that previously kept a request alive by dripping header bytes will now time out after the idle window (default 300 s / BUN_CONFIG_HTTP_IDLE_TIMEOUT / per-request timeout). That matches undici's headersTimeout, and {timeout: false} still opts out, but a human should sign off on the semantic shift.
Other factors
Four prior review rounds on this PR have already been folded in (doc comment sync, comment length, test wall-clock 18s→8s via concurrent cases + 3-byte body, dead BodyChunk-arm re-arm dropped, dead receive_paused conjunct replaced with debug_assert!). The author gave a reasonable explanation for not adding a dedicated test for the :3731 re-arm (uSockets' 4 s sweep tick makes the delta phase-dependent and non-deterministic under ~12 s), and reasonably declined CodeRabbit's three suggestions with reference to existing suite conventions. CI build #83625 is in flight.
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ... # Conflicts: # src/jsc/bindings/BunDebugger.cpp
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ...
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ... # Conflicts: # src/js/internal/debugger.ts
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ...
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 theshort_read!path ofhandle_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 aton_open); only the drip defeats it.Reproduction
The same shape with no bytes written (silent stall) already rejects with
TimeoutError: The operation timed out.on the existing build, soAbortSignal.timeout()is the only way to bound the drip case today.Change
set_timeoutcall fromshort_read!inhandle_on_data_headers. The timer stays as armed byon_open/on_writable, so it is an absolute deadline for the header block (undiciheadersTimeoutsemantics).on_datare-arm onresponse_stage == Body | BodyChunk, mirroring the non-proxy dispatch, so the same deadline holds for HTTPS through a CONNECT proxy.handle_response_metadatasucceeds so the body phase starts with a fresh idle window rather than whatever was left of the header deadline (folded from fetch: make the response-header phase an absolute deadline #36146). Body reads continue to re-arm per chunk (undicibodyTimeoutsemantics).IDLE_TIMEOUT_SECONDSdoc comment to describe the new header-phase behaviour.The default deadline is unchanged (300 s /
BUN_CONFIG_HTTP_IDLE_TIMEOUT/ per-requesttimeout), and{timeout: false}still disables it.Verification
New test in
test/js/web/fetch/fetch.test.ts: a rawnet.Serverdrips 10 header bytes at 2 s each against{timeout: 5000}and must reject withTimeoutError, then drips a 5-byte body after a burst header block and must resolve with 200.bun-install-stalled-tls.test.ts,fetch-keepalive.test.ts, the adjacent "explicit numeric `timeout` extends the socket idle deadline" test, andproxy.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-requesttimeoutas per-request options with no change to the header-phase re-arm; this change is independent and composes with it. AheadersTimeoutoption distinct from the body-phase idle value can follow once the per-request plumbing in #33338 lands.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