fetch: let an explicit timeout extend the socket idle deadline - #33647
Merged
Conversation
fetch(url, { timeout: ms }) previously only distinguished "disabled"
(0/false) from "enabled": any positive value still left the socket on the
global 300s idle timer (BUN_CONFIG_HTTP_IDLE_TIMEOUT), so a request that
legitimately goes quiet for longer than 5 minutes aborted even when the
caller asked for a longer deadline.
Thread the millisecond value (when finite and > 0) through FetchOptions ->
AsyncHTTP options -> HTTPClient.idle_timeout_seconds, and have the h1
set_timeout and h2 rearm_timeout paths use the per-request value when
present (for a shared h2 session, the longest among attached streams).
timeout: 0/false still disables the timer entirely, and non-finite or
non-positive numbers keep today's behavior. The uSockets normalisation
(clamp to 239 min, round >240s up to a whole minute) moves into a shared
normalize_idle_timeout_seconds() used by both the env default and the
per-request override.
Fixes #16682
Contributor
|
Warning Review limit reached
Next review available in: 3 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)
WalkthroughThis PR adds a per-request idle timeout option to fetch. A ChangesPer-request idle timeout support
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Collaborator
|
Updated 8:17 PM PT - Jul 7th, 2026
❌ @robobun, your commit 9972962 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33647That installs a local version of the PR into your bun-33647 --bun |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
alii
reviewed
Jul 7, 2026
Collaborator
|
✅ All review follow-ups landed through 9972962. |
… stream shares it
rearm_timeout computes the shared socket's idle deadline as the max of
every attached client's effective_idle_timeout_seconds(). A
disable_timeout client contributes 0 there, so a sibling's short
explicit override (e.g. {timeout:1000}) armed the socket at 1s and
on_long_timeout -> session.on_close failed every stream on the session,
including the one whose caller passed {timeout:false}. Before
per-request overrides the same mix armed at the global default, so the
no-timeout stream had that as a floor.
Track whether any attached client has disable_timeout and, when one
coexists with siblings that want a timer, floor the max at the global
idle_timeout_seconds() so the no-timeout stream keeps the same lower
bound it had before. When every client opted out the max is still 0 and
the timer stays disarmed.
Test covers both halves of the h2 aggregate: a long explicit override
extends the session past a tiny global default, and a {timeout:false}
stream survives a sibling's 1s explicit override.
…:false} stream is present; declare fetch() timeout option in BunFetchRequestInit When BUN_CONFIG_HTTP_IDLE_TIMEOUT=0, idle_timeout_seconds() is 0 and want.max(0) is a no-op, so the floor added in 9782070 gave no protection: a {timeout:false} h2 stream was still armed at (and killed by) a coalesced sibling's short explicit override. Pre-per-request- override the same mix armed at idle_timeout_seconds()=0 -> set_timeout(0) = disarmed; restore that by disarming explicitly when any_disabled and the global default is 0. Covered by a third scenario in the h2 aggregate test (global=0, {timeout:false} + {timeout:1000} both survive a 10s hold). Also declare timeout?: number | boolean on BunFetchRequestInit so the now-meaningful numeric semantics are discoverable.
…onds()==0, not just disable_timeout
A plain fetch() (no timeout option) under BUN_CONFIG_HTTP_IDLE_TIMEOUT=0
has disable_timeout=false and idle_timeout_seconds=None, so its
effective_idle_timeout_seconds() is 0 ("no timeout") but the previous
any_disabled gate keyed only on the flag and missed it. A coalesced
sibling with a short explicit {timeout:N} then armed the shared socket
and on_close failed both, the same class already fixed for explicit
{timeout:false} in 9782070/dd30609.
Key the gate on the computed value instead so any client whose effective
deadline is "none" is protected regardless of how it got there. The
global=0 test scenario now includes a plain fetch alongside the explicit
{timeout:false} to cover both.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http/h2_client/ClientSession.rs (1)
514-550: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the aggregation into a small helper for readability.
rearm_timeoutnow mixes traversal (streams + pending_attach), a stateful closure, and the "unbounded floor" post-processing in one function. A small helper likefn effective_session_timeout(&self) -> c_uintreturning(want, any_unbounded)(or the final clamped value) would separate "compute" from "apply" and make the floor logic easier to unit-test in isolation.🤖 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 514 - 550, The timeout re-arming logic in rearm_timeout is doing both aggregation over streams/pending_attach and the final global-floor adjustment in one place, which hurts readability and testability. Extract the computation into a small helper such as effective_session_timeout (or a similar private method) that returns the final clamped timeout or the intermediate state needed for clamping, then keep rearm_timeout focused on calling self.socket.set_timeout with that result. Preserve the current behavior for stream_ref, pending_client_mut, and crate::idle_timeout_seconds while moving the “any_unbounded”/floor logic into the helper.
🤖 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 `@packages/bun-types/globals.d.ts`:
- Around line 2088-2092: The fetch timeout behavior and the `timeout` docs are
inconsistent: `fetch(url, { timeout: NaN })` and `Infinity` currently disable
the idle timer because `fetch` parsing in `src/runtime/webcore/fetch.rs` treats
`timeout.to_int32() == 0` as disabled, while `packages/bun-types/globals.d.ts`
says non-finite values use the default. Update the `fetch` timeout parser to
treat non-finite values as the default deadline, or adjust the
`RequestInit.timeout` docs in `globals.d.ts` to match the actual
`fetch`/`RequestInit` behavior.
In `@test/js/web/fetch/fetch-http2-client.test.ts`:
- Around line 1968-1976: The test leaves parent-side hold timers pending after
early child exit, which can keep the process alive and leak callbacks across
tests. Update the relevant fetch HTTP/2 test cases around makeH2Server,
server.close(), and the 10s setTimeout usage to track each pending timer and
clear them during cleanup. Wrap the test flow in try/finally so the timer
cleanup and server shutdown always run, even when assertions fail.
In `@test/js/web/fetch/fetch.test.ts`:
- Around line 2887-2939: Move the idle-timeout regression out of fetch.test.ts
into a new standalone fetch regression file under test/js/web/fetch/ following
the fetch-*.test.ts pattern. Keep the existing scenario around the explicit
numeric timeout behavior, the Bun.spawn subprocess, and the
BUN_CONFIG_HTTP_IDLE_TIMEOUT-dependent setup, but relocate the test so the
monolithic fetch suite stays smaller and this config-heavy multi-second case is
isolated. Use the same test body and helpers like Bun.serve, Bun.spawn, and the
timeout assertions, just place them in a dedicated scenario-specific test file.
---
Outside diff comments:
In `@src/http/h2_client/ClientSession.rs`:
- Around line 514-550: The timeout re-arming logic in rearm_timeout is doing
both aggregation over streams/pending_attach and the final global-floor
adjustment in one place, which hurts readability and testability. Extract the
computation into a small helper such as effective_session_timeout (or a similar
private method) that returns the final clamped timeout or the intermediate state
needed for clamping, then keep rearm_timeout focused on calling
self.socket.set_timeout with that result. Preserve the current behavior for
stream_ref, pending_client_mut, and crate::idle_timeout_seconds while moving the
“any_unbounded”/floor logic into the helper.
🪄 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: a1d7fff1-b9b9-40df-980e-5666de85cfa6
📒 Files selected for processing (9)
packages/bun-types/globals.d.tssrc/http/AsyncHTTP.rssrc/http/HTTPThread.rssrc/http/h2_client/ClientSession.rssrc/http/lib.rssrc/runtime/webcore/fetch.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/js/web/fetch/fetch-http2-client.test.tstest/js/web/fetch/fetch.test.ts
… hold timers in h2 idle-deadline test cleanup The parser's to_int32()==0 path means NaN/Infinity disable the idle timer (ToInt32 of non-finite is 0), so document that rather than claiming non-finite uses the default. Track the per-request hold timers in the h2 aggregate test and clear them in finally so an early child exit does not leave 10s callbacks pending past server.close().
JSValue::to_int32() is JSC's coerceJSValueDoubleTruncatingT, which
saturates +/-Infinity to i32::MAX/MIN (not spec ToInt32's 0), so
{timeout: Infinity} previously fell through to the global default
instead of meaning "never time out". Gate the disable decision on
!ms.is_finite() as well so non-finite values uniformly disable, making
the BunFetchRequestInit.timeout JSDoc accurate and giving Infinity the
intuitive semantics. Covered by a new withInfinity case in the h1
idle-deadline test.
robobun
added a commit
that referenced
this pull request
Jul 9, 2026
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fetch(url, { timeout: 60 * 60 * 1000 })(or a longAbortSignal.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 numerictimeoutwas 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
timeoutoption: when it is a finite number > 0, the millisecond value is converted to seconds and threaded as a per-requestidle_timeout_secondsthroughFetchOptions->AsyncHTTPoptions ->HTTPClient, andHTTPClient::effective_idle_timeout_seconds()(used by both the h1set_timeoutpath and the h2 session'srearm_timeout, which arms the longest deadline among the streams sharing the connection) prefers it over the global default.timeout: 0/falsestill 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 sharednormalize_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