fetch: add connectTimeout, socketTimeout, and a whole-request timeout - #33338
fetch: add connectTimeout, socketTimeout, and a whole-request timeout#33338robobun wants to merge 2 commits into
Conversation
|
Updated 1:30 PM PT - Jul 9th, 2026
❌ @robobun, your commit 3baeef9 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 33338That installs a local version of the PR into your bun-33338 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds structured ChangesFetch connect/idle timeout support
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/runtime/webcore/fetch.rs`:
- Around line 207-210: The doc comment above the socket idle timer logic in
fetch.rs exceeds the 3-line comment limit. Condense the explanation near the
idle timer rounding logic so it stays within three lines while preserving the
key point in the existing comment block around the timer sweep behavior.
In `@src/runtime/webcore/fetch/FetchTasklet.rs`:
- Around line 2287-2345: Reset the connect timeout state on each redirect hop in
FetchTasklet so a new TCP/TLS dial is not treated as already connected by
inherited Signals.connected. Update the redirect/reset path to clear or
reinitialize the connected flag before re-queuing the next request, and make
sure arm_connect_timeout and on_connect_timeout still govern the per-hop
deadline correctly. If the intended behavior is whole-request scope instead,
document that explicitly in the timeout/redirect flow.
In `@test/js/web/fetch/fetch-timeout-options.test.ts`:
- Around line 51-76: The test in fetch-timeout-options.test.ts includes a
vacuous stderr assertion that checks for no "panic", which should be removed.
Update the stalled-connect test around the Bun.spawn/fetch path to rely on
concrete assertions already present, such as the exact stdout string and
exitCode from the proc result, and drop the stderr containment check entirely or
replace it with a meaningful assertion tied to the observed timeout 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: c9e0e56c-3e54-4dff-9379-97b0a271e3dc
📒 Files selected for processing (18)
docs/runtime/networking/fetch.mdxpackages/bun-types/globals.d.tssrc/event_loop/EventLoopTimer.rssrc/http/AsyncHTTP.rssrc/http/HTTPContext.rssrc/http/HTTPThread.rssrc/http/Signals.rssrc/http/h2_client/ClientSession.rssrc/http/h3_client/ClientSession.rssrc/http/h3_client/callbacks.rssrc/http/lib.rssrc/http_types/FetchRedirect.rssrc/jsc/bindings/ErrorCode.cppsrc/jsc/bindings/webcore/AbortSignal.hsrc/runtime/dispatch.rssrc/runtime/webcore/fetch.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/js/web/fetch/fetch-timeout-options.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/webcore/fetch/FetchTasklet.rs (1)
825-830: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the new timeout comments within the 3-line limit.
These changed comment blocks exceed the repo rule; the same intent fits in shorter comments.
Proposed cleanup
- // The request settled. `clear_data()` would get here eventually, but - // the tasklet outlives the settle by a turn of the event loop - // (`deinit` is re-dispatched to the JS thread), and a deadline firing - // in that window would abort an already-finished request. + // The tasklet outlives settlement by one event-loop turn; cancel now + // so a deadline cannot abort an already-finished request.- /// The `timeout.connect` deadline expired. Only a request still dialling - /// (no socket yet, or a TLS handshake that never completed) is cancelled; - /// once the transport is up the deadline is meaningless and the socket's - /// idle timer takes over. - /// - /// One-shot, so the deadline covers the first connection only. `Signals - /// .connected` is never cleared (see `HTTPClient::mark_connected`), so a - /// redirect that reopens a connection is not re-bounded. + /// Abort if `timeout.connect` expires before the first connection completes. + /// After `Connected` is set, the socket idle timer owns timeout handling; + /// redirects/retries are not re-bounded because this deadline is one-shot.As per coding guidelines, "Keep code comments to 3 lines max".
Also applies to: 2314-2321
🤖 Prompt for 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. In `@src/runtime/webcore/fetch/FetchTasklet.rs` around lines 825 - 830, The new timeout comment block in FetchTasklet::deinit exceeds the repo’s 3-line comment limit and should be shortened while preserving the same intent. Reword the explanatory comment near is_done/cancel_connect_timeout() into a compact 1–3 line version, and apply the same cleanup to the other affected comment block referenced in the review so the timeout rationale stays clear without extra lines.Source: Coding guidelines
src/http/lib.rs (1)
291-299: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCondense the new timeout doc comments to the 3-line limit.
Lines 291-299 and 3941-3951 exceed the repo’s comment-length rule; keep the durable invariant, but trim the prose.
Proposed cleanup
-/// Fit `raw` seconds to what the uSockets timers can actually represent. -/// -/// uSockets' long-timeout counter is `% 240` minutes (`us_socket_long_timeout` -/// in packages/bun-usockets/src/socket.c), so values above 239 min wrap around -/// and fire early. Above 240s `socket.set_timeout` floors to whole minutes, so -/// round up first — otherwise the armed timer is *shorter* than requested. -/// Shared by the process-wide `BUN_CONFIG_HTTP_IDLE_TIMEOUT` (normalised once -/// in `HTTPThread::on_start`) and the per-request `fetch({ timeout: { idle } })` -/// override, so both arming paths see an already-representable value. +/// Normalize idle timeouts to uSockets' representable range: max 239 minutes. +/// Values above 240s are rounded up to whole minutes so the armed timer is +/// never shorter than requested.- /// The transport is usable and the request is about to go out: TCP - /// connected, TLS handshaken for https, or a pooled socket handed straight - /// to us. This is where `timeout.connect` stops applying and the socket's - /// idle timer takes over, so it must be reached on *every* path that can - /// send a request — a path that forgets it spuriously times out a healthy - /// connection. - /// - /// Write-once by design: `timeout.connect` bounds the request's *first* - /// connection, not a later one reopened by `do_redirect` or a retry. Making - /// it per-hop would need the HTTP thread to tell the JS thread to re-arm its - /// deadline on every new dial, which no existing channel carries. + /// Mark the transport usable: TCP connected, TLS handshaken, or pooled. + /// This stops `timeout.connect` for the request's first connection; redirects + /// and retries are not re-bounded because no re-arm channel exists today.As per coding guidelines, "Keep code comments to 3 lines max".
Also applies to: 3941-3951
🤖 Prompt for 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. In `@src/http/lib.rs` around lines 291 - 299, The timeout documentation comment in the HTTP idle-timeout path is too long and must be condensed to fit the repo’s 3-line comment limit. Trim the prose in the comment near the timer normalization logic while preserving the key invariant about uSockets’ representable timeout range and the shared normalization used by HTTPThread::on_start and fetch timeout handling. Apply the same shortening to the other duplicated timeout comment block so both remain brief and consistent.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/http/lib.rs`:
- Around line 291-299: The timeout documentation comment in the HTTP
idle-timeout path is too long and must be condensed to fit the repo’s 3-line
comment limit. Trim the prose in the comment near the timer normalization logic
while preserving the key invariant about uSockets’ representable timeout range
and the shared normalization used by HTTPThread::on_start and fetch timeout
handling. Apply the same shortening to the other duplicated timeout comment
block so both remain brief and consistent.
In `@src/runtime/webcore/fetch/FetchTasklet.rs`:
- Around line 825-830: The new timeout comment block in FetchTasklet::deinit
exceeds the repo’s 3-line comment limit and should be shortened while preserving
the same intent. Reword the explanatory comment near
is_done/cancel_connect_timeout() into a compact 1–3 line version, and apply the
same cleanup to the other affected comment block referenced in the review so the
timeout rationale stays clear without extra lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c84a860-9064-4d9a-80e3-200dbe518cc2
📒 Files selected for processing (4)
src/http/lib.rssrc/runtime/webcore/fetch.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/js/web/fetch/fetch-timeout-options.test.ts
|
The two outside-diff findings are addressed in 37f331b. Those comments were flagged because the previous commit added code-level documentation of the connect-timeout scope (which this review asked for), and that ran past the 3-line limit. Rather than fix only the two that were flagged, I swept the whole diff: every comment block it adds is now within the limit. Nine blocks were trimmed, in
|
There was a problem hiding this comment.
Both nits from my last pass are addressed in fb3aa1c (the shared cancel_request_body helper and the AbortSignal.rs discriminant mapping) and I found nothing new — deferring only because this adds public API surface and threads a new timer through FetchTasklet across all three transports, which a maintainer should sign off on.
Extended reasoning...
Overview
This PR adds fetch({ timeout: { connect, idle } }), fixing #16682 (numeric timeout was a no-op) and adding a first-class connect deadline. It touches 19 files: fetch option parsing (fetch.rs), a new EventLoopTimer embedded in FetchTasklet with arm/cancel/fire lifecycle, a new Signals.connected atomic set from the HTTP thread at every transport-established point (h1 first_call, h2 adopt/enqueue/attach, h3 on_hsk_done, and the pooled-proxy-tunnel branch in HTTPContext), a per-request idle_timeout_seconds override plumbed through AsyncHTTP/HTTPClient with the h2 session's rearm_timeout reworked to take the max across multiplexed clients, a new CommonAbortReason::ConnectionTimeout variant across Rust/C++, docs, types, and 14 tests.
Since my last review, fb3aa1c extracted cancel_request_body(reason) so the connect-timeout abort path now cancels a streaming request body the same way abort_listener does, and added the explicit 3 => / 4 => arms plus a debug_assert! catch-all in AbortSignal::reason_if_aborted. Both of my prior findings are resolved; the bug hunter found nothing new on this revision.
Security risks
None identified. The new option only tightens deadlines (no new network reach, no auth/crypto surface). Input validation rejects negative/NaN/fractional values and clamps to u32::MAX. The connected atomic is a monotonic write-once flag used only to suppress an abort, so a missed mark_connected() call degrades to a spurious timeout rather than anything unsafe.
Level of scrutiny
High. This is production-critical HTTP client code with cross-thread coordination: a JS-thread EventLoopTimer node is embedded in a heap-allocated FetchTasklet and must be unlinked from the VM timer heap before the tasklet is freed. The PR handles this carefully (clear_data() calls cancel_connect_timeout() first, on_progress_update disarms on is_done, impl_timer_owner! provides the field-offset recovery), and the h2 rearm_timeout refactor changes shared-socket semantics. It also introduces new user-facing API whose shape (object vs. scalar, false vs. 0, whole-request vs. per-hop connect scope) is a design decision a maintainer should ratify.
Other factors
Test coverage is thorough (positive, negative-contract, argument validation, subprocess isolation for env-var overrides), the PR description documents deliberate limits and a regression sweep against main, and all prior review threads (CodeRabbit and mine) are resolved. My deferral is purely about scope — new public API + timer lifecycle in a hot path — not about any outstanding defect.
CI status: the diff is green, the red lanes are infraSummarising three builds so this doesn't have to be re-derived.
The job dies before the test runner starts, so it cannot be reacting to anything in this diff. It is also flaky per-shard rather than deterministic: on build 68402 one
That build ran on 42e2ae4, which changed zero functional lines of A no-op commit cannot change how Where each lane actually stands
x64-asan is the lane that would catch a problem with the new I'm not pushing a Ready for reviewAll review threads from CodeRabbit and claude[bot] are resolved, including two real findings in the last round: the connect-timeout abort path now cancels a streaming request body (fetch spec §abort-fetch step 5) via a shared |
|
Build 68422 has now finished, which closes out the analysis above: 284 jobs passed, 2 failed, and both failures are the same The darwin-14 lanes that were still queued when I wrote the comment above have since gone green. Not re-rolling: that shard has failed 3 of the 4 times it has run across these builds, so a fresh build has roughly a 1-in-16 chance of both shards clearing, and a retry doesn't fix an agent-side download timeout anyway. Happy to push one if a maintainer would rather see a fully green run. The diff itself is green everywhere it actually executed, including all 20 |
There was a problem hiding this comment.
The implementation looks solid and every prior finding has been addressed, but this adds new public API surface to fetch() (timeout: { connect, idle, total }, plus a semantic change to what a bare timeout: N means) and embeds two EventLoopTimer nodes in the cross-thread FetchTasklet — both the API shape and the timer lifecycle deserve a maintainer's eyes before this lands.
Extended reasoning...
Overview
This PR adds per-phase timeouts to fetch(): connect (DNS+TCP+TLS), idle (socket inactivity), and — new since my last review, in 0c0826e — total (whole-request wall-clock). It touches 19 files across the HTTP client stack (src/http/lib.rs, h1/h2/h3 client sessions, Signals, HTTPThread), the event loop (EventLoopTimer, dispatch.rs), the fetch entry point (fetch.rs, FetchTasklet.rs), C++ abort-reason plumbing (ErrorCode.cpp, AbortSignal.h), and adds docs, types, and a 15-test file. All prior review threads (mine and CodeRabbit's) are resolved: the vacuous panic assertion, the 4-line comments, the missing request-body cancellation in on_connect_timeout, and the stale CommonAbortReason reverse mapping have all been addressed. The bug hunter found nothing this round.
Security risks
None material. Timeout values are validated (NaN/negative/fractional rejected, clamped to u32::MAX), the new atomic Signals.connected is write-once from the HTTP thread and read-only from the JS thread with acquire/release ordering, and no user-controlled input reaches anything security-sensitive. The change does not touch TLS verification, credential handling, or path parsing.
Level of scrutiny
High. This is not a mechanical change:
- New public API design. The shape of
{ connect, idle, total }, and especially what a baretimeout: Nmeans, is a user-facing contract. 0c0826e changed that from "sets both connect and idle" (still what the PR description says) to "means{ total: N }" (what the code and docs now say). That's a reasonable choice — it matches Go'sClient.Timeoutand reqwest — but it's exactly the kind of decision.claude/docs/landing-prs.mdreserves for maintainer agreement. - Memory-safety-sensitive lifecycle. Two
EventLoopTimernodes are now embedded inFetchTasklet, which is refcounted and freed after cross-thread completion. The PR handles this carefully (cancel_timeouts()at the top ofclear_data()and onis_doneinon_progress_update()), and theimpl_timer_owner!container-recovery pattern matches existing usage, but the interaction between the JS-thread timer heap and theThreadSafeRefCountteardown is subtle enough to warrant a maintainer's read. - Cross-transport behavior change.
h2_client::rearm_timeoutmoved from a boolean any-client-wants-timeout to a max-of-all-clients aggregation. That's semantically different for multiplexed streams and worth a second opinion.
Other factors
Test coverage is good (positive, negative, and validation cases; verified fail-on-system-Bun), CI is green everywhere it actually ran, and the author has been thorough and responsive to every review comment. The PR description is slightly out of date relative to HEAD (it predates total and still describes the old scalar semantics). This is a well-executed feature PR — it just isn't in the "simple/mechanical/obvious" category that's safe to approve without a human.
robobun
changed the title
The API shape is settled, and it changed@claude[bot] deferred on exactly the right thing: the option shape was a design decision, not a mechanical one. It has since been made. Two maintainers weighed in, and the result is a different (better) API than the one reviewed above, so the PR title, body, docs and types have all been rewritten to match. Summarising, so nobody reviews the old shape: Three flat sibling options, no nested object: timeout?: boolean | number; // whole-request wall-clock deadline
connectTimeout?: number | false; // DNS + TCP + the TLS handshake
socketTimeout?: number | false; // inactivity in either directionThree things moved since the last review:
On the other two concerns raised: the
|
CI status: diff green where it runs,
|
|
Build 69293 has now finished: 281 jobs passed, 3 failed, all three on darwin-aarch64.
My one self-serve re-roll is spent, so I'm not pushing again. Ready for a maintainer; a merge re-runs CI on fresh agents. |
3adc6a3 to
55de3ef
Compare
|
Rebased onto main (conflict-free, diff unchanged) to restart CI. But I don't think this fixes #16682 yet, and I'd hold it until it does. #16682's reproduction is
Given this also adds new user-facing option names ( |
|
You're right, and thanks for the precise trace — that's exactly the gap. Fixed in 1dd3797. A numeric total_timeout_ms = ms;
// #16682: the caller's deadline must not be preempted by the
// default socket-idle timer, so raise it to match. An explicit
// `socketTimeout` still overrides this below.
idle_timeout_seconds = Some(ms_to_idle_timeout_seconds(ms));And the reproduction test you described, proven load-bearing by ablation (deleting the one-line fix makes it fail): // https://github.com/oven-sh/bun/issues/16682
test("a numeric timeout longer than the socket-idle default is respected", async () => {
// Process-wide socket-idle default is 1s; the request asks for 10 minutes.
// Without the fix, the idle default preempts the caller's deadline at ~1–5s.
await using proc = Bun.spawn({
cmd: [bunExe(), "-e",
`const r = await fetch(${JSON.stringify(server.url.href)}, { timeout: 600_000 });
console.log(await r.text());`],
env: { ...bunEnv, BUN_CONFIG_HTTP_IDLE_TIMEOUT: "1" },
...On On splitting: happy to do it either way.
Let me know which you'd prefer. |
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 `@docs/runtime/networking/fetch.mdx`:
- Around line 223-240: Update the `socketTimeout` note in the `fetch()` docs to
mention the undocumented upper clamp: very large values are silently capped by
the runtime (the `normalize_idle_timeout_seconds` behavior) to just under 240
minutes to avoid timer-wheel wraparound. Add a brief sentence near the existing
rounding/granularity explanation so users know oversized `socketTimeout` values
will be truncated rather than honored exactly.
In `@packages/bun-types/globals.d.ts`:
- Around line 1930-1965: Clarify the `FetchInit.timeout` documentation so
`@default true` no longer reads as contradictory to the prose in `globals.d.ts`.
Reword the `timeout` JSDoc to explicitly state that the default/unset value is
`true`, which means “use the built-in timeout behavior without a whole-request
deadline,” while numeric values set a whole-request deadline and `false`/`0`
disable all timeouts. Keep the behavior description aligned within the `timeout`
property block.
In `@src/http/h2_client/ClientSession.rs`:
- Around line 520-533: The rearm_timeout logic in ClientSession currently uses
max() across active streams and pending attachments, which incorrectly delays
socket idle expiration to the longest request timeout. Update rearm_timeout to
drive the socket from the earliest active idle deadline instead, and make sure
it still handles the shared H2 socket semantics in ClientSession without
breaking longer-lived siblings; if a single timeout value cannot express this
safely, add per-stream idle tracking or equivalent earliest-deadline scheduling
around stream_ref(...).client_ref(), pending_client_mut(...), and
self.socket.set_timeout().
🪄 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: 2f252f6c-9995-4144-b100-c278657c2bf5
📒 Files selected for processing (19)
docs/runtime/networking/fetch.mdxpackages/bun-types/globals.d.tssrc/event_loop/EventLoopTimer.rssrc/http/AsyncHTTP.rssrc/http/HTTPContext.rssrc/http/HTTPThread.rssrc/http/Signals.rssrc/http/h2_client/ClientSession.rssrc/http/h3_client/ClientSession.rssrc/http/h3_client/callbacks.rssrc/http/lib.rssrc/http_types/FetchRedirect.rssrc/jsc/AbortSignal.rssrc/jsc/bindings/ErrorCode.cppsrc/jsc/bindings/webcore/AbortSignal.hsrc/runtime/dispatch.rssrc/runtime/webcore/fetch.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/js/web/fetch/fetch-timeout-options.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/http/h2_client/ClientSession.rs (1)
524-534: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
disable_timeoutinrearm_timeout()
effective_idle_timeout_seconds()returns0for disabled requests, butmax()treats that as a no-op, so any attached or pending client withdisable_timeoutcan still inherit a sibling’s finite session timeout and be closed early. Track a disabled participant separately and disarm the socket timer when one is present.🤖 Prompt for 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. In `@src/http/h2_client/ClientSession.rs` around lines 524 - 534, The timeout rearm logic in rearm_timeout currently only uses max over effective_idle_timeout_seconds(), so a client with disable_timeout can still inherit a nonzero session timeout from other streams or pending attaches. Update ClientSession::rearm_timeout to track whether any stream or pending client has disable_timeout enabled (via stream_ref(...).client_ref() and pending_client_mut(...)) and, if so, disarm or clear the socket timeout instead of calling self.socket.set_timeout with a finite value; otherwise keep the existing max-based timeout behavior.src/runtime/webcore/fetch.rs (1)
936-944: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDocument that a numeric
timeoutsilently raises the effectivesocketTimeout.This new branch raises
idle_timeout_secondsto matchtotal_timeout_mswhenever a numerictimeoutis supplied, so the caller's whole-request deadline isn't preempted by the default 5-minute (orBUN_CONFIG_HTTP_IDLE_TIMEOUT) idle timer — this is the intended#16682fix.However, neither
docs/runtime/networking/fetch.mdxnorpackages/bun-types/globals.d.tsmentions this. Both currently documenttimeoutandsocketTimeoutas fully independent axes, withsocketTimeoutdefaulting to a fixed "5 minutes"/300_000. A caller who sets{ timeout: 900_000 }(15 min) without touchingsocketTimeoutwill now get their idle-quiet protection silently relaxed from 5 minutes to 15 minutes to match — which is a meaningful, currently-undocumented change to the "no bytes moving" safety net callers may be relying on.Recommend adding a sentence to the
<Note>block infetch.mdxand to thetimeout/socketTimeoutJSDoc inglobals.d.tsclarifying that an explicit numerictimeoutraises the effectivesocketTimeoutto match unlesssocketTimeoutis also set explicitly.🤖 Prompt for 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. In `@src/runtime/webcore/fetch.rs` around lines 936 - 944, Document the interaction between numeric timeout and socketTimeout in fetch: the current fetch runtime branch in fetch.rs intentionally raises idle_timeout_seconds to match total_timeout_ms when timeout is a number, so callers do not assume the default idle timer stays at 5 minutes. Update the <Note> in docs/runtime/networking/fetch.mdx and the timeout/socketTimeout JSDoc in packages/bun-types/globals.d.ts to state that an explicit numeric timeout silently raises the effective socketTimeout unless socketTimeout is also provided explicitly. Refer to the fetch timeout handling and the timeout/socketTimeout API docs so the change is easy to locate.
🤖 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.
Outside diff comments:
In `@src/http/h2_client/ClientSession.rs`:
- Around line 524-534: The timeout rearm logic in rearm_timeout currently only
uses max over effective_idle_timeout_seconds(), so a client with disable_timeout
can still inherit a nonzero session timeout from other streams or pending
attaches. Update ClientSession::rearm_timeout to track whether any stream or
pending client has disable_timeout enabled (via stream_ref(...).client_ref() and
pending_client_mut(...)) and, if so, disarm or clear the socket timeout instead
of calling self.socket.set_timeout with a finite value; otherwise keep the
existing max-based timeout behavior.
In `@src/runtime/webcore/fetch.rs`:
- Around line 936-944: Document the interaction between numeric timeout and
socketTimeout in fetch: the current fetch runtime branch in fetch.rs
intentionally raises idle_timeout_seconds to match total_timeout_ms when timeout
is a number, so callers do not assume the default idle timer stays at 5 minutes.
Update the <Note> in docs/runtime/networking/fetch.mdx and the
timeout/socketTimeout JSDoc in packages/bun-types/globals.d.ts to state that an
explicit numeric timeout silently raises the effective socketTimeout unless
socketTimeout is also provided explicitly. Refer to the fetch timeout handling
and the timeout/socketTimeout API docs so the change is easy to locate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 240c4738-ab53-4b59-bda8-8494d37984cf
📒 Files selected for processing (5)
docs/runtime/networking/fetch.mdxpackages/bun-types/globals.d.tssrc/http/h2_client/ClientSession.rssrc/runtime/webcore/fetch.rstest/js/web/fetch/fetch-timeout-options.test.ts
|
Both outside-diff findings addressed in 6da956d. h2 On whether this regresses from
|
) `fetch(url, { timeout: 60 * 60 * 1000 })` (or a long `AbortSignal.timeout`) still aborted after ~5 minutes: Bun arms a socket idle timer on every HTTP client socket, defaulting to 300 seconds (`BUN_CONFIG_HTTP_IDLE_TIMEOUT`), and a numeric `timeout` was only ever inspected for "is it 0/false" to decide whether to disable that timer. Any positive value fell through to the global default, so slow responses (LLM providers, long polls) died at 5 minutes no matter what deadline the caller asked for. That behavior was wrong because the *default* idle timer silently preempted an *explicit* user deadline: the option existed, was accepted, and then had no effect on when the request was killed. Callers who wanted more than 5 minutes had to disable Bun's timeout entirely and re-implement their own. The fix stays inside the existing `timeout` option: when it is a finite number > 0, the millisecond value is converted to seconds and threaded as a per-request `idle_timeout_seconds` through `FetchOptions` -> `AsyncHTTP` options -> `HTTPClient`, and `HTTPClient::effective_idle_timeout_seconds()` (used by both the h1 `set_timeout` path and the h2 session's `rearm_timeout`, which arms the longest deadline among the streams sharing the connection) prefers it over the global default. `timeout: 0`/`false` still means "no timeout", and non-finite/non-positive values keep today's behavior. The uSockets normalisation (clamp to 239 minutes, round values above 240s up to a whole minute) is factored into a shared `normalize_idle_timeout_seconds()` used by both the env default and the per-request override. This intentionally adds no new option names; #33338 (granular connect/socket timeouts) is the broader API proposal and can layer on top of this plumbing. Fixes #16682 --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
Squashed and rebased onto #33647, which landed the per-request idle_timeout_seconds plumbing (HTTPClient/AsyncHTTP/FetchOptions, effective_idle_timeout_seconds, normalize_idle_timeout_seconds, and the h2 rearm_timeout aggregation) along with the #16682 fix that a numeric timeout extends the socket-idle deadline. This PR keeps what is net-new on top of that: timeout whole-request wall-clock deadline (EventLoopTimer) connectTimeout DNS + TCP + TLS socketTimeout socket inactivity in either direction plus the supporting pieces: Signals.connected and mark_connected() across h1/h2/h3 so connectTimeout knows when dialling ended; the two embedded EventLoopTimers in FetchTasklet with dispatch, arm, cancel and on-fire handling; CommonAbortReason::ConnectionTimeout so connectTimeout reports a distinct message; the cancel_request_body() helper shared by every abort path so a streaming request body is cancelled with the reason; docs, types, and a 21-test suite. Conflict resolution against #33647: took main for every shared hunk (normalize/effective helpers, h2 rearm_timeout, AsyncHTTP::init normalisation site). Two adjustments followed: - timeout_ms_arg now treats Infinity as "no deadline", matching the semantic #33647 established and tests for. - The h2 idle-aggregation test's "short explicit deadline" sub-cases switched {timeout: 1000} -> {socketTimeout: 1000}, since timeout now arms a per-request wall-clock deadline that is independent of the shared socket timer the test is exercising.
6da956d to
bef0c74
Compare
…err assertions
The rearm_timeout doc comment was describing the pre-rebase implementation
(disarm entirely when any client is unbounded), which the rebase discarded in
favour of main's floor-at-global-default behaviour. Since this PR no longer
touches the function body, it should not touch the doc comment either; reverted
to main's version so the function drops out of the diff.
The two .not.toContain("TimeoutError") assertions were the same dead-assertion
pattern as the .not.toContain("panic") removed in 42e2ae4: the preceding
.toEqual({stdout, exitCode}) already fails first on the regression they guard
against, so the stderr check is never reached.
|
CI on the rebased sha (3baeef9, build 71122): 281 passed, x64-asan 20/20 green, none of this PR's test files in any annotation. The one failure is Re-roll already spent; not pushing again. |
…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 -->
2 participants
fetch()has one timeout for everything, which forces callers to pick a single number that has to be generous enough for the slowest response body they expect. That same number then also governs how long a request spends failing to open a socket on a bad network. Splitting the two is whathttpx,requests,reqwest, OkHttp and undici all do; this adds the equivalent to Bun.Two problems turned up while implementing it.
timeout: <number>was silently a no-opOnly
falseand0ever did anything (they disarmed the socket idle timer). Every other number was read, coerced, and thrown away:This is #16682, open since February. The option is also absent from
@types/bun, so the only way to discover it is from the issue tracker.There was no connect timeout at all
The idle timer is armed in
on_open, so DNS resolution and the TCP handshake are uncovered. A socket stuck inSYN_SENThangs until the kernel exhausts its SYN retransmits, around two minutes on Linux:ss -tanpconfirms the client socket sits inSYN-SENTthe whole time.AbortSignalis the only escape, and it can't distinguish "could not connect" from "the body is still streaming".What changed
Three deadlines, as flat sibling options, which is how every surveyed HTTP client spells them:
timeoutfetch()until the body finishesconnectTimeoutsocketTimeoutAll three are milliseconds.
connectTimeoutrejects withTimeoutError: The connection timed out., so a retry policy can tell "the network is down" from "the server is slow"; the other two reject withTimeoutError: The operation timed out.socketTimeoutandtimeoutanswer different questions, and callers often want both.socketTimeoutfires when the connection goes quiet, and is re-armed on every byte, so a response that trickles in steadily never trips it.timeoutfires regardless of activity, and is the one that catches a server that is technically alive but far too slow.timeout: false(or0) disables every timeout for the request, which is the long-poll/SSE escape hatch people found in #16682, so it keeps working. An explicitsocketTimeoutoutranks it, so{ timeout: false, socketTimeout: 30_000 }does what it reads like.Why these three axes, and why `timeout` is the whole-request one: a survey of 16 HTTP clients across 11 languages
Every value below was read from the library's source, not from docs or recollection.
requeststimeout[0]timeout[1], read-onlyhttpxconnect5sread5s +write5saiohttpsock_connect30ssock_read, read-onlytotal300snet/httpTimeout30s +TLSHandshakeTimeout10sClient.Timeoutreqwestconnect_timeoutread_timeout, read-onlytimeoutOkHttpconnectTimeout10sreadTimeout10s +writeTimeout10scallTimeoutKtorconnectTimeoutMillissocketTimeoutMillis, bidirectionalrequestTimeoutMillisjava.net.httpconnectTimeoutHttpRequest.timeoutundiciconnectTimeout10sbodyTimeout300s, read-onlyaxiostimeout, bidirectional (node)timeout(browser only)gotlookup+connect+secureConnectsocket, bidirectionalrequest.NETConnectTimeout, infiniteHttpClient.Timeout100sNet::HTTPopen_timeout60sread_timeout60s +write_timeout60sGuzzleconnect_timeout300sread_timeout, stream handler onlytimeoutlibcurlCURLOPT_CONNECTTIMEOUT300sCURLOPT_TIMEOUTURLSessiontimeoutIntervalForRequest60s, bidirectionaltimeoutIntervalForResource7dThree things this settles:
requests,httpx,undiciandNet::HTTP, which happen to be the clients the original two-axis version of this PR was modelled on.URLSession,gotand axios-on-node all re-arm on bytes in either direction. Splitting read from write (OkHttp, httpx, Ruby) only distinguishes a stalled upload, which a read-only timer misses entirely. Soidlestays bidirectional, and there is no separateread.timeoutis the whole-request one wherever that axis exists. Go, reqwest, .NET, libcurl, Guzzle andgot'srequestall read a bare number that way.Closest prior art: Ktor ships this exact trio (
connectTimeoutMillis/socketTimeoutMillis/requestTimeoutMillis), andURLSessionships the inactivity + whole-request split (timeoutIntervalForRequestis documented as "reset whenever data is transmitted";timeoutIntervalForResourceis the hard deadline).socketTimeouttakes its name from Ktor andgot, the two clients with the same bidirectional semantics.idleTimeoutwas the other candidate, butBun.serve({ idleTimeout })is in seconds, so a millisecondidlesitting next to it would have been a units trap.How
connectis a one-shotEventLoopTimerembedded inFetchTasklet, armed inqueue(). The HTTP thread sets a newconnectedatomic on the existingSignals::StoreatHTTPClient::first_call, the single funnel every transport reaches before writing a request (on_openfor plain TCP,on_handshakefor TLS, plus the h2 session attach/adopt/enqueue, h3'son_hsk_done, and the pooled-proxy-tunnel branch that skipsfirst_call). If the deadline expires while that atomic is still false, the timer reuses the existing abort-during-connect path to tear the socket down.us_timer_twould have been the obvious home for this on the HTTP thread, butsrc/uws_sys/Timer.rsmarks it deprecated (one timerfd per timer on Linux), and uSockets' sweep never walkshead_connecting_sockets, so a DNS-deferred connect is invisible to it anyway. The JS-thread timer is the same mechanismAbortSignal.timeoutalready uses to escape a stalled connect, so it has millisecond precision and no new teardown path.timeoutis a secondEventLoopTimeron the tasklet with the same lifecycle as the connect deadline, but no phase check: it fires in whatever phase the request is in, which is exactly what makes it catch a server that is alive but too slow. Both deadlines are dropped together bycancel_timeouts(), whichclear_data()always reaches.socketTimeoutbecomes a per-request override ofBUN_CONFIG_HTTP_IDLE_TIMEOUTvia a newHTTPClient::idle_timeout_seconds.HTTPClient::set_timeoutand h2'sClientSession::rearm_timeoutboth route through a neweffective_idle_timeout_seconds(); the h2 path, which shares one socket across multiplexed streams, now arms the longest timeout any attached client asked for instead of a boolean, and still disarms only when every client opted out. The clamp-and-round logic thatHTTPThread::on_startapplied to the env var moves into a sharednormalize_idle_timeout_secondsso both assignment sites produce a value the uSockets timer can represent.Deliberate limits
connectTimeoutbounds the initial connection. A redirect that reopens a connection is not re-bounded;connectedis sticky for the life of the request.connectTimeoutandtimeoutare measured fromfetch(), so time spent queued behindBUN_CONFIG_MAX_HTTP_REQUESTScounts against them.socketTimeoutinherits the socket timer's coarse resolution (4s ticks, 60s above 240s) and can fire up to one tick early. That is pre-existing behaviour ofBUN_CONFIG_HTTP_IDLE_TIMEOUT, now just reachable per-request. Documented as a backstop, not a precise deadline.bun installandnode:httptoo, which deserves its own PR (undici defaults to 10s).Rebase onto #33647
#33647 landed the per-request
idle_timeout_secondsplumbing (HTTPClient/AsyncHTTP/FetchOptions,effective_idle_timeout_seconds,normalize_idle_timeout_seconds, and the h2rearm_timeoutaggregation) along with the #16682 fix that a numerictimeoutextends the socket-idle deadline. This PR now carries only what is net-new on top of it, squashed into one commit.Conflict resolution took main for every shared hunk. Two adjustments followed:
timeout_ms_argnow treatsInfinityas "no deadline", matching the semantic fetch: let an explicittimeoutextend the socket idle deadline #33647 established and tests for.{timeout: 1000}to{socketTimeout: 1000}, sincetimeoutnow arms a per-request wall-clock deadline that is independent of the shared socket timer that test exercises.Verification
test/js/web/fetch/fetch-timeout-options.test.ts, 21 tests. The connect cases use a raw TCP listener that accepts and never answers the ClientHello, the same hermetic trickbun-install-stalled-tls.test.tsalready uses:first_callis never reached, which is exactly the state a dropped SYN produces, but deterministic enough to assert on. Several tests are negative contracts (the deadline must not fire on a healthy-but-slow request, onconnect: 0, or once connected).Regression sweep
Every suite below was run against a debug build of this branch and against a debug build of
main, and the failing sets are identical. The failures that do exist are environmental (the container has no route to TEST-NET-1, an intercepting HTTP proxy, and debug+ASAN timing assertions).fetch.test.tsmainfetch-http2-client.test.tsfetch-http3-client.test.tsfetch-keepalive.test.tsfetch-abort-queued/-stream-body/-socket-close-racebun-install-stalled-tls.test.tsfetch-leak.test.ts,abort-signal-leak.test.ts,fetch-preconnect.test.tsmainbun-types.test.tsNotes
EventLoopTimer::Tagand a field toFetchTasklet, so whichever lands second needs a trivial rebase.CommonAbortReasongains aConnectionTimeoutvariant so the two timeouts are distinguishable by message while keeping the web-standardTimeoutErrorshape.[review] gate passed · iteration 10 · 18 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 10
evidence per changed file