From bef0c747932d56eb7a8d68749042f66f7d8cea4d Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 9 Jul 2026 18:26:00 +0000 Subject: [PATCH 1/2] fetch: add connectTimeout, socketTimeout, and a whole-request timeout 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. --- docs/runtime/networking/fetch.mdx | 76 +++- packages/bun-types/globals.d.ts | 92 +++++ src/event_loop/EventLoopTimer.rs | 2 + src/http/HTTPContext.rs | 4 + src/http/Signals.rs | 9 + src/http/h2_client/ClientSession.rs | 16 +- src/http/h3_client/ClientSession.rs | 11 + src/http/h3_client/callbacks.rs | 1 + src/http/lib.rs | 13 + src/http_types/FetchRedirect.rs | 1 + src/jsc/AbortSignal.rs | 7 +- src/jsc/bindings/ErrorCode.cpp | 3 + src/jsc/bindings/webcore/AbortSignal.h | 1 + src/runtime/dispatch.rs | 13 + src/runtime/webcore/fetch.rs | 149 ++++++-- src/runtime/webcore/fetch/FetchTasklet.rs | 164 ++++++++- test/js/web/fetch/fetch-http2-client.test.ts | 14 +- .../web/fetch/fetch-timeout-options.test.ts | 327 ++++++++++++++++++ 18 files changed, 843 insertions(+), 60 deletions(-) create mode 100644 test/js/web/fetch/fetch-timeout-options.test.ts diff --git a/docs/runtime/networking/fetch.mdx b/docs/runtime/networking/fetch.mdx index afe904b75e09..46a991ebbb5f 100644 --- a/docs/runtime/networking/fetch.mdx +++ b/docs/runtime/networking/fetch.mdx @@ -166,14 +166,84 @@ When using streams with S3: ### Fetching a URL with a timeout -To fetch a URL with a timeout, use `AbortSignal.timeout`: +The simplest deadline is `timeout`, a number of milliseconds: the whole request +must finish within it, or it rejects. ```ts -const response = await fetch("http://example.com", { - signal: AbortSignal.timeout(1000), +// Fails if the request isn't completely done within 5 seconds. +await fetch("http://example.com", { timeout: 5_000 }); +``` + +One deadline for everything is a blunt instrument, though. A large download +legitimately takes minutes, but nothing should spend minutes opening a socket. +Two more options bound the individual phases. + +```ts +const response = await fetch("http://example.com/large-file", { + // Give up if the connection isn't established within 5 seconds. + connectTimeout: 5_000, + // ...but let the download take as long as it needs, as long as bytes keep + // arriving at least every 2 minutes. + socketTimeout: 120_000, }); ``` +| Option | Covers | Default | +| ---------------- | ---------------------------------------------------------------------- | ----------- | +| `timeout` | The whole request, from `fetch()` until the body finishes. | No deadline | +| `connectTimeout` | DNS resolution, the TCP handshake, and the TLS handshake for `https:`. | No deadline | +| `socketTimeout` | Any stretch where no bytes move in either direction, in any phase. | 5 minutes | + +All three are in milliseconds and all three reject with a `TimeoutError`. +`connectTimeout` reports `The connection timed out.`, so a retry policy can tell +"the network is down" from "the server is slow"; the other two report +`The operation timed out.` + +`socketTimeout` and `timeout` answer different questions, and you often want +both: + +- `socketTimeout` fires when the connection goes quiet. It is re-armed on every + byte in either direction, so a response that trickles in steadily never trips + it. +- `timeout` fires regardless of activity. It is the one that catches a server + that is technically alive but far too slow. + +```ts +// Bail if the server goes quiet for 30s, AND never spend more than 2 minutes. +await fetch("http://example.com/", { socketTimeout: 30_000, timeout: 120_000 }); +``` + +`timeout: false` (or `0`) disables _every_ timeout for the request, which is what +you want for long-polling and server-sent events: + +```ts +await fetch("http://example.com/events", { timeout: false }); +``` + + + +`connectTimeout` applies to the initial connection, not to one reopened while +following a redirect. It and `timeout` are both measured from the moment +`fetch()` is called, so time spent queued behind the [simultaneous connection +limit](#simultaneous-connection-limit) counts against them. + +`socketTimeout` is rounded up to whole seconds (and to whole minutes above 240 +seconds) because that is the resolution of the underlying socket timer, which may +also fire up to one tick early, and values above 239 minutes are clamped to that +ceiling. Treat it as a coarse backstop rather than a precise deadline; `timeout` +and `connectTimeout` have millisecond precision. Its default can be changed +process-wide with the `BUN_CONFIG_HTTP_IDLE_TIMEOUT` environment variable, in +seconds. + +A numeric `timeout` also raises the effective `socketTimeout` to match unless +`socketTimeout` is set explicitly, so the socket-idle default can never preempt +the deadline you wrote. `{ timeout: 900_000, socketTimeout: 30_000 }` keeps both +bounds; `{ timeout: 900_000 }` alone relaxes the idle backstop to 15 minutes. +Setting `socketTimeout` explicitly outranks `timeout: false`, so +`{ timeout: false, socketTimeout: 30_000 }` keeps the socket timer you asked for. + + + #### Canceling a request To cancel a request, use an `AbortController`: diff --git a/packages/bun-types/globals.d.ts b/packages/bun-types/globals.d.ts index d88036b35841..bb14b3e082a2 100644 --- a/packages/bun-types/globals.d.ts +++ b/packages/bun-types/globals.d.ts @@ -1927,6 +1927,98 @@ interface BunFetchRequestInit extends RequestInit { */ tls?: BunFetchRequestInitTLS; + /** + * Milliseconds the whole request may take, measured from the moment `fetch()` + * is called until the response body has finished. A hard wall-clock deadline: + * it fires even while bytes are actively arriving, so a server that trickles + * data forever is still cut off. Rejects with a + * `TimeoutError: The operation timed out.` + * + * The equivalent of `Client.Timeout` in Go, `timeout` in reqwest and + * `libcurl`, `callTimeout` in OkHttp, and `timeoutIntervalForResource` in + * `URLSession`. + * + * Defaults to `true`, which means "use the other timeout defaults with no + * whole-request deadline": {@link socketTimeout} is still on, but there is no + * overall wall-clock limit unless you pass a number here. + * + * A number also raises the effective {@link socketTimeout} to at least that + * value unless `socketTimeout` is set explicitly, so the socket-idle default + * can never fire ahead of the deadline you wrote. + * + * `false` (or `0`) disables *every* timeout for this request, including + * {@link socketTimeout}. + * + * Not part of the Fetch API specification. + * + * @default true + * + * @example + * ```js + * // The whole request must finish within 5 seconds. + * const response = await fetch("https://example.com/", { timeout: 5_000 }); + * ``` + * + * @example + * ```js + * // Never time out: useful for long-polling or server-sent events. + * const response = await fetch("https://example.com/events", { timeout: false }); + * ``` + */ + timeout?: boolean | number; + + /** + * Milliseconds to spend establishing the connection (DNS resolution, the TCP + * handshake, and the TLS handshake for `https:`) before the request rejects + * with a `TimeoutError: The connection timed out.` The distinct message lets a + * retry policy tell "the network is down" from "the server is slow". + * + * Applies to the initial connection, not to one reopened while following a + * redirect. Measured from the moment `fetch()` is called, so time spent queued + * behind `BUN_CONFIG_MAX_HTTP_REQUESTS` in-flight requests counts against it. + * + * `0` or `false` means no connect deadline. + * + * Not part of the Fetch API specification. + * + * @default false + * + * @example + * ```js + * // Fail fast on a dead network, but allow a slow response body. + * await fetch("https://example.com/large", { + * connectTimeout: 5_000, + * socketTimeout: 120_000, + * }); + * ``` + */ + connectTimeout?: number | false; + + /** + * Milliseconds the socket may sit with no bytes moving in either direction + * before the request rejects with a `TimeoutError: The operation timed out.` + * The timer is re-armed on every read and every write, so a response that + * trickles in steadily never trips it. Use {@link timeout} when you need a + * deadline that fires regardless of activity. + * + * This is `socketTimeoutMillis` in Ktor and `socket` in `got`. It is the + * equivalent of `read`/`bodyTimeout` elsewhere, except that it also covers a + * stalled upload. + * + * Rounded up to whole seconds (and to whole minutes above 240 seconds) + * because that is the resolution of the underlying socket timer, which may + * also fire up to one tick early. It is a coarse backstop, not a precise + * deadline. + * + * `0` or `false` means no socket deadline. Setting it explicitly outranks + * `timeout: false`. + * + * Not part of the Fetch API specification. + * + * @default 300_000 (overridable process-wide with `BUN_CONFIG_HTTP_IDLE_TIMEOUT`, in seconds) + */ + socketTimeout?: number | false; + /** * Log the raw HTTP request and response to stdout, as a debugging aid. * This API may be removed in a future version of Bun without notice. diff --git a/src/event_loop/EventLoopTimer.rs b/src/event_loop/EventLoopTimer.rs index c2397060c68b..8705fb3c01b8 100644 --- a/src/event_loop/EventLoopTimer.rs +++ b/src/event_loop/EventLoopTimer.rs @@ -199,6 +199,8 @@ pub enum Tag { DevServerSweepSourceMaps, DevServerMemoryVisualizerTick, AbortSignalTimeout, + FetchConnectTimeout, + FetchTotalTimeout, DateHeaderTimer, BunTest, EventLoopDelayMonitor, diff --git a/src/http/HTTPContext.rs b/src/http/HTTPContext.rs index d5db29f77305..6829a2be53c2 100644 --- a/src/http/HTTPContext.rs +++ b/src/http/HTTPContext.rs @@ -1011,6 +1011,10 @@ impl HTTPContext { // the centralised [`proxy_tunnel::raw_as_mut`] backref upgrade. crate::proxy_tunnel::raw_as_mut(raw).adopt::(client, sock); client.on_open::(sock)?; + // With `SSL` this branch never reaches `first_call` (the tunnel + // is established and `on_open` only calls it for plain TCP), so + // mark the connect phase done here. Redundant when `!SSL`. + client.mark_connected(); client.on_writable::(sock); } else { client.on_open::(sock)?; diff --git a/src/http/Signals.rs b/src/http/Signals.rs index e8a01a85ff96..55641254d154 100644 --- a/src/http/Signals.rs +++ b/src/http/Signals.rs @@ -10,6 +10,10 @@ pub struct Signals { pub aborted: Option>, pub cert_errors: Option>, pub upgraded: Option>, + /// Set once the transport is established (h1 socket open, h2 session adopted, + /// h3 handshake done). Read from the JS thread by the `connectTimeout` + /// deadline to tell "still dialing" from "connected". + pub connected: Option>, pub body_receive_mode: Option>, } @@ -59,6 +63,7 @@ impl Signals { Field::Aborted => self.aborted, Field::CertErrors => self.cert_errors, Field::Upgraded => self.upgraded, + Field::Connected => self.connected, }?; Some(bun_ptr::BackRef::from(ptr)) } @@ -89,6 +94,7 @@ pub struct Store { pub aborted: AtomicBool, pub cert_errors: AtomicBool, pub upgraded: AtomicBool, + pub connected: AtomicBool, pub body_receive_mode: AtomicU8, } @@ -100,6 +106,7 @@ impl Default for Store { aborted: AtomicBool::new(false), cert_errors: AtomicBool::new(false), upgraded: AtomicBool::new(false), + connected: AtomicBool::new(false), body_receive_mode: AtomicU8::new(BodyReceiveMode::AutoPause as u8), } } @@ -113,6 +120,7 @@ impl Store { aborted: Some(NonNull::from(&self.aborted)), cert_errors: Some(NonNull::from(&self.cert_errors)), upgraded: Some(NonNull::from(&self.upgraded)), + connected: Some(NonNull::from(&self.connected)), body_receive_mode: None, } } @@ -156,4 +164,5 @@ pub enum Field { Aborted, CertErrors, Upgraded, + Connected, } diff --git a/src/http/h2_client/ClientSession.rs b/src/http/h2_client/ClientSession.rs index d9696a577cf4..976b7d3bc62b 100644 --- a/src/http/h2_client/ClientSession.rs +++ b/src/http/h2_client/ClientSession.rs @@ -299,6 +299,9 @@ impl ClientSession { pub fn adopt(&mut self, client: &mut HTTPClient) { client.h2_register_abort_tracker(self.socket); + // The session's socket is already established, so this request is past + // the connect phase even while it waits for SETTINGS in `pending_attach`. + client.mark_connected(); // Park instead of attaching when (a) we're inside onData's deliver // loop — attach() mustn't mutate `streams` under iteration — or (b) // the server's first SETTINGS hasn't arrived yet, so the real @@ -332,6 +335,7 @@ impl ClientSession { /// is routed via the session socket so `abortByHttpId` can find it. pub fn enqueue(&mut self, client: &mut HTTPClient<'_>) { client.h2_register_abort_tracker(self.socket); + client.mark_connected(); self.pending_attach.push(client.as_erased_ptr().as_ptr()); self.rearm_timeout(); } @@ -397,6 +401,7 @@ impl ClientSession { /// DATA, and flush. pub fn attach(&mut self, client: &mut HTTPClient) { debug_assert!(self.has_headroom()); + client.mark_connected(); let send_window = i32::try_from(self.remote_initial_window_size.min(wire::MAX_WINDOW_SIZE)) .expect("int cast"); @@ -511,12 +516,11 @@ impl ClientSession { } } - /// Re-arm the shared socket's idle timer based on the aggregate of every - /// attached client. With multiplexed streams the per-request - /// `disable_timeout` flag can't drive the socket directly (last writer - /// would win and a `{timeout:false}` long-poll could be killed by a - /// sibling re-arming, or strip the safety net from one that wants it), - /// so the session disarms only when *every* attached client opted out. + /// Re-arm the shared socket's idle timer at the longest any attached client + /// asked for — treating `0` ("disarm") as unbounded, not as a minimum. The + /// fire handler kills the whole session, so a shorter value would cut off a + /// longer-timeout or `timeout: false` sibling; this only delays the short + /// one. Per-stream idle tracking would be the exact answer. fn rearm_timeout(&mut self) { // The socket is shared by every stream on the session, so arm the // longest effective idle timeout among them (0 = every client's diff --git a/src/http/h3_client/ClientSession.rs b/src/http/h3_client/ClientSession.rs index d4b0c6e8513d..4d9fb1e73534 100644 --- a/src/http/h3_client/ClientSession.rs +++ b/src/http/h3_client/ClientSession.rs @@ -115,11 +115,22 @@ impl ClientSession { self.ref_(); if self.handshake_done { + client.mark_connected(); // handshake_done implies qsocket is Some and valid. self.qsocket_mut().unwrap().make_stream(); } } + /// The QUIC handshake landed: every request parked on this connection is + /// past the connect phase, so `connectTimeout` must stop applying to them. + pub fn mark_pending_connected(&mut self) { + for &stream_ptr in self.pending.iter() { + if let Some(client) = stream_mut(stream_ptr).client { + client_mut(client).mark_connected(); + } + } + } + pub fn stream_body_by_http_id(&mut self, async_http_id: u32, ended: bool) { for &stream_ptr in self.pending.iter() { let stream = stream_mut(stream_ptr); diff --git a/src/http/h3_client/callbacks.rs b/src/http/h3_client/callbacks.rs index f9d8b23bc419..3421c417a773 100644 --- a/src/http/h3_client/callbacks.rs +++ b/src/http/h3_client/callbacks.rs @@ -101,6 +101,7 @@ extern "C" fn on_hsk_done(qs: *mut quic::Socket, ok: c_int) { return; } session.handshake_done = true; + session.mark_pending_connected(); for _ in 0..session.pending.len() { qs.make_stream(); } diff --git a/src/http/lib.rs b/src/http/lib.rs index 3c12b4afdba6..df9ce34cdfc7 100644 --- a/src/http/lib.rs +++ b/src/http/lib.rs @@ -1963,6 +1963,10 @@ impl<'a> HTTPClient<'a> { } pub fn first_call(&mut self, socket: HttpSocket) { + // Reached from `on_open` for plain TCP and from `on_handshake` for TLS, + // so `connectTimeout` covers DNS + TCP + the TLS handshake. + self.mark_connected(); + if FeatureFlags::IS_FETCH_PRECONNECT_SUPPORTED { if self.flags.is_preconnect_only { self.on_preconnect::(socket); @@ -4115,6 +4119,15 @@ impl<'a> HTTPClient<'a> { socket.set_timeout(self.effective_idle_timeout_seconds()); } + /// Transport usable (TCP connected, TLS handshaken, or pooled): where + /// `connectTimeout` stops applying. Must be reached on *every* path that can + /// send a request; write-once, so only the first connection is bounded. + #[inline] + pub fn mark_connected(&self) { + self.signals + .store(signals::Field::Connected, true, Ordering::Release); + } + fn maybe_pause_receive(&mut self, socket: HttpSocket) { if self.state.flags.receive_paused || self.proxy_tunnel.is_some() diff --git a/src/http_types/FetchRedirect.rs b/src/http_types/FetchRedirect.rs index b05e7b418df3..4b9199e3f4f4 100644 --- a/src/http_types/FetchRedirect.rs +++ b/src/http_types/FetchRedirect.rs @@ -34,4 +34,5 @@ pub enum CommonAbortReason { Timeout = 1, UserAbort = 2, ConnectionClosed = 3, + ConnectionTimeout = 4, } diff --git a/src/jsc/AbortSignal.rs b/src/jsc/AbortSignal.rs index 1c32228b8fae..dce56ffd4bd0 100644 --- a/src/jsc/AbortSignal.rs +++ b/src/jsc/AbortSignal.rs @@ -135,7 +135,12 @@ impl AbortSignal { return Some(AbortReason::Common(match reason { 1 => CommonAbortReason::Timeout, 2 => CommonAbortReason::UserAbort, - _ => CommonAbortReason::ConnectionClosed, + 3 => CommonAbortReason::ConnectionClosed, + 4 => CommonAbortReason::ConnectionTimeout, + _ => { + debug_assert!(false, "unknown CommonAbortReason discriminant {reason}"); + CommonAbortReason::ConnectionClosed + } })); } if js_reason.is_empty() { diff --git a/src/jsc/bindings/ErrorCode.cpp b/src/jsc/bindings/ErrorCode.cpp index abe3a18f09b0..bafec052ca2b 100644 --- a/src/jsc/bindings/ErrorCode.cpp +++ b/src/jsc/bindings/ErrorCode.cpp @@ -1782,6 +1782,9 @@ JSC::JSValue WebCore::toJS(JSC::JSGlobalObject* globalObject, CommonAbortReason case CommonAbortReason::ConnectionClosed: { return createDOMException(globalObject, ExceptionCode::AbortError, "The connection was closed."_s); } + case CommonAbortReason::ConnectionTimeout: { + return createDOMException(globalObject, ExceptionCode::TimeoutError, "The connection timed out."_s); + } default: { break; } diff --git a/src/jsc/bindings/webcore/AbortSignal.h b/src/jsc/bindings/webcore/AbortSignal.h index a41aeebac944..0ed266292c55 100644 --- a/src/jsc/bindings/webcore/AbortSignal.h +++ b/src/jsc/bindings/webcore/AbortSignal.h @@ -54,6 +54,7 @@ enum class CommonAbortReason : uint8_t { Timeout, UserAbort, ConnectionClosed, + ConnectionTimeout, }; JSC::JSValue toJS(JSC::JSGlobalObject*, CommonAbortReason); diff --git a/src/runtime/dispatch.rs b/src/runtime/dispatch.rs index 9ddfc8c0454b..41a5a4304871 100644 --- a/src/runtime/dispatch.rs +++ b/src/runtime/dispatch.rs @@ -1048,6 +1048,19 @@ pub unsafe fn __bun_fire_timer(t: *mut EventLoopTimer, now: *const ElTimespec, v timer_arm!(Subprocess<'_>, event_loop_timer, |c, _now, _vm| (*c) .timeout_callback()) } + EventLoopTimerTag::FetchConnectTimeout => { + // SAFETY: §Dispatch — tag set together with the container in + // `FetchTasklet::get`; `t` is the tasklet's `connect_timeout_timer`. + let container = unsafe { FetchTasklet::from_connect_timeout_timer_ptr(t) }; + // SAFETY: per fn contract. + unsafe { (*container).on_connect_timeout() }; + } + EventLoopTimerTag::FetchTotalTimeout => { + // SAFETY: §Dispatch — `t` is the tasklet's `total_timeout_timer`. + let container = unsafe { FetchTasklet::from_total_timeout_timer_ptr(t) }; + // SAFETY: per fn contract. + unsafe { (*container).on_total_timeout() }; + } EventLoopTimerTag::DevServerSweepSourceMaps => { // `sweep_weak_refs` takes the raw `*EventLoopTimer` and recovers // the store inside. diff --git a/src/runtime/webcore/fetch.rs b/src/runtime/webcore/fetch.rs index 7bda0212bc0f..f73256faf404 100644 --- a/src/runtime/webcore/fetch.rs +++ b/src/runtime/webcore/fetch.rs @@ -180,6 +180,66 @@ impl HTTPRequestBodyExt for HTTPRequestBody { } } +// ────────────────────────────────────────────────────────────────────────── +// timeout +// ────────────────────────────────────────────────────────────────────────── + +/// A `fetch({ timeout })` duration in milliseconds. `0` and `false` both mean +/// "no deadline". `name` is the user-facing option (`timeout`, `connectTimeout`, +/// ...) so the error echoes back exactly what they wrote. +fn timeout_ms_arg(global_this: &JSGlobalObject, value: JSValue, name: &str) -> JsResult { + if value.is_boolean() && !value.as_boolean() { + return Ok(0); + } + if value.is_number() { + let n = value.as_number(); + // `Infinity` means "no deadline", same as `false`/`0`. + if n == f64::INFINITY { + return Ok(0); + } + if !n.is_nan() && n >= 0.0 && n.fract() == 0.0 { + // Above u32::MAX a timeout is indistinguishable from "never", and + // `Timespec::ms_from_now` takes an i64 — clamp instead of wrapping. + return Ok(n.min(f64::from(u32::MAX)) as u32); + } + } + Err(global_this.throw_invalid_arguments(format_args!( + "fetch: '{name}' must be a non-negative integer number of milliseconds, or false" + ))) +} + +/// Read a `number | false` millisecond option off the first init object that +/// carries it. `None` means the caller never set it. +fn optional_timeout_ms( + global_this: &JSGlobalObject, + objects: &[JSValue], + name: &str, +) -> JsResult> { + for obj in objects { + if obj.is_empty() { + continue; + } + let Some(value) = obj.get(global_this, name)? else { + continue; + }; + if value.is_undefined_or_null() { + continue; + } + return Ok(Some(timeout_ms_arg(global_this, value, name)?)); + } + Ok(None) +} + +/// uSockets' idle sweep only understands whole seconds. Round up so the armed +/// timer is never *shorter* than asked; a sub-second value becomes 1 second, +/// not 0 (which would disarm it entirely). +fn ms_to_idle_timeout_seconds(ms: u32) -> core::ffi::c_uint { + if ms == 0 { + return 0; + } + http::normalize_idle_timeout_seconds(u64::from(ms).div_ceil(1000)) +} + // ────────────────────────────────────────────────────────────────────────── // dataURLResponse // ────────────────────────────────────────────────────────────────────────── @@ -437,6 +497,8 @@ fn fetch_impl( let first_arg = args.next_eat().unwrap(); let mut disable_timeout = false; + let mut connect_timeout_ms: u32 = 0; + let mut total_timeout_ms: u32 = 0; let mut idle_timeout_seconds: Option = None; let mut disable_keepalive = false; let mut disable_decompression = false; @@ -864,46 +926,75 @@ fn fetch_impl( } } - // timeout: false | number | undefined - disable_timeout = 'extract_disable_timeout: { + // timeout: boolean | number | undefined — a deadline on the whole request. + // `false`/`0` disarm every timeout for the request, which is the escape hatch + // long-polling and SSE callers already rely on. + 'extract_timeout: { let objects_to_try = [ options_object.unwrap_or(JSValue::ZERO), request_init_object.unwrap_or(JSValue::ZERO), ]; for obj in objects_to_try { - if !obj.is_empty() { - if let Some(timeout_value) = obj.get(global_this, "timeout")? { - if timeout_value.is_boolean() { - break 'extract_disable_timeout !timeout_value.as_boolean(); - } else if timeout_value.is_number() { - // A finite positive `timeout` (in ms) also governs the - // socket idle deadline, overriding the global - // `BUN_CONFIG_HTTP_IDLE_TIMEOUT` default. - let ms = timeout_value.as_number(); - if ms.is_finite() && ms > 0.0 { - idle_timeout_seconds = - Some((ms / 1000.0).ceil().min(core::ffi::c_uint::MAX as f64) - as core::ffi::c_uint); - } - // `to_int32()` saturates ±Infinity (JSC's - // `coerceJSValueDoubleTruncatingT`, not spec ToInt32), - // so gate on `is_finite()` too so `{timeout: Infinity}` - // disables the timer instead of silently falling back - // to the global default. - break 'extract_disable_timeout !ms.is_finite() - || timeout_value.to_int32() == 0; - } - } - + if obj.is_empty() { + continue; + } + let Some(timeout_value) = obj.get(global_this, "timeout")? else { if global_this.has_exception() { return Ok(JSValue::ZERO); } + continue; + }; + + if timeout_value.is_boolean() { + disable_timeout = !timeout_value.as_boolean(); + break 'extract_timeout; + } + + if timeout_value.is_number() { + let ms = timeout_ms_arg(global_this, timeout_value, "timeout")?; + if ms == 0 { + disable_timeout = true; + } else { + 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)); + } + break 'extract_timeout; + } + + if !timeout_value.is_undefined_or_null() { + return Err(global_this.throw_invalid_arguments(format_args!( + "fetch: 'timeout' must be a boolean or a non-negative integer number of milliseconds" + ))); } } + } - break 'extract_disable_timeout disable_timeout; - }; + if global_this.has_exception() { + return Ok(JSValue::ZERO); + } + + { + let objects_to_try = [ + options_object.unwrap_or(JSValue::ZERO), + request_init_object.unwrap_or(JSValue::ZERO), + ]; + + // connectTimeout: number | false | undefined + if let Some(ms) = optional_timeout_ms(global_this, &objects_to_try, "connectTimeout")? { + connect_timeout_ms = ms; + } + // socketTimeout: number | false | undefined. An explicit value outranks + // `timeout: false`, so `{ timeout: false, socketTimeout: 30_000 }` keeps + // the socket timer the caller asked for. + if let Some(ms) = optional_timeout_ms(global_this, &objects_to_try, "socketTimeout")? { + idle_timeout_seconds = Some(ms_to_idle_timeout_seconds(ms)); + disable_timeout = false; + } + } if global_this.has_exception() { return Ok(JSValue::ZERO); @@ -2060,6 +2151,8 @@ fn fetch_impl( body, disable_keepalive, disable_timeout, + connect_timeout_ms, + total_timeout_ms, idle_timeout_seconds, disable_decompression, max_redirects, diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index c97257ce9993..dd61d8eefafa 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -5,6 +5,7 @@ use bun_boringssl as boringssl; use bun_cares_sys::c_ares_draft as c_ares; use bun_core::{Error as BunError, err}; use bun_core::{MutableString, OwnedString, String as BunString, ZigStringSlice}; +use bun_event_loop::EventLoopTimer::{EventLoopTimer, State as TimerState, Tag as TimerTag}; use bun_event_loop::{ AnyTask::AnyTask, ConcurrentTask::{AutoDeinit, ConcurrentTask}, @@ -20,7 +21,8 @@ use bun_io::KeepAlive; use bun_jsc::debugger::AsyncTaskTracker; use bun_jsc::virtual_machine::VirtualMachine; use bun_jsc::{ - self as jsc, GlobalRef, JSGlobalObject, JSValue, JsResult, StringJsc, StrongOptional, + self as jsc, CommonAbortReasonExt as _, GlobalRef, JSGlobalObject, JSValue, JsResult, + StringJsc, StrongOptional, }; use bun_sys::FdExt; use bun_threading::Mutex; @@ -125,9 +127,24 @@ pub struct FetchTasklet { pub tracker: AsyncTaskTracker, + /// `fetch({ connectTimeout })`. Armed in `queue()`, unlinked in + /// `clear_data()` — the only path that frees the tasklet, so the timer heap + /// never holds a dangling node. Never inserted when no connect timeout is set. + pub connect_timeout_timer: EventLoopTimer, + + /// `fetch({ timeout })`. Same lifecycle as the connect deadline, + /// but fires whatever phase the request is in: it bounds the whole request, + /// body included, so byte activity never re-arms it. + pub total_timeout_timer: EventLoopTimer, + pub ref_count: bun_ptr::ThreadSafeRefCount, } +bun_event_loop::impl_timer_owner!(FetchTasklet; + from_connect_timeout_timer_ptr => connect_timeout_timer, + from_total_timeout_timer_ptr => total_timeout_timer, +); + // Boxing `AnyBlob` is not viable: the `AnyBlob` arm is constructed/matched in // `fetch.rs` (e.g. `HTTPRequestBodyExt::any_blob`) and would require changes // across files. The enum is also short-lived per-request, so the size cost is bounded. @@ -446,6 +463,9 @@ impl FetchTasklet { fn clear_data(&mut self) { bun_output::scoped_log!(FetchTasklet, "clearData "); + // Before anything else: a live heap node pointing into a box we are + // about to free is a use-after-free on the next timer sweep. + self.cancel_timeouts(); if !self.url_proxy_buffer.is_empty() { self.url_proxy_buffer = Box::default(); } @@ -809,6 +829,12 @@ impl FetchTasklet { self.mutex.lock(); self.has_schedule_callback.store(false, Ordering::Relaxed); let is_done = !self.result.has_more; + if is_done { + // The tasklet outlives settlement by an event-loop turn (`deinit` is + // re-dispatched to the JS thread); a deadline firing in that window + // would abort an already-finished request. + self.cancel_timeouts(); + } let vm = self.javascript_vm; // vm is shutting down we cannot touch JS @@ -1882,6 +1908,8 @@ impl FetchTasklet { // SAFETY: jsc_vm derived from FFI ptr above; AsyncTaskTracker::init only // bumps a counter on the VM. tracker: AsyncTaskTracker::init(global_this.bun_vm().as_mut()), + connect_timeout_timer: EventLoopTimer::init_paused(TimerTag::FetchConnectTimeout), + total_timeout_timer: EventLoopTimer::init_paused(TimerTag::FetchTotalTimeout), ref_count: bun_ptr::ThreadSafeRefCount::init(), }); @@ -2080,24 +2108,29 @@ impl FetchTasklet { #[bun_uws::uws_callback] pub(crate) fn abort_listener(&mut self, reason: JSValue) { bun_output::scoped_log!(FetchTasklet, "abortListener"); - let this = self; reason.ensure_still_alive(); - this.abort_reason.set(&this.global_this, reason); - this.abort_task(); - if let Some(sink) = this.sink_mut() { + self.abort_reason.set(&self.global_this, reason); + self.abort_task(); + self.cancel_request_body(reason); + } + + /// https://fetch.spec.whatwg.org/#abort-fetch step 5: a still-readable request + /// body is cancelled with the abort reason, so the underlying source's + /// `cancel(reason)` observes it. Shared by every abort path. + fn cancel_request_body(&mut self, reason: JSValue) { + if let Some(sink) = self.sink_mut() { sink.cancel(reason); return; } - // Abort fired before the HTTP thread asked for the body, so the - // ReadableStream was never wired into a sink. Cancel it directly so - // the underlying source's cancel(reason) callback still observes the - // signal's reason (https://fetch.spec.whatwg.org/#abort-fetch step 5). - if this.is_waiting_request_stream_start { - if let HTTPRequestBody::ReadableStream(stream_ref) = &this.request_body { - this.is_waiting_request_stream_start = false; - if let Some(stream) = stream_ref.get(&this.global_this) { - stream.cancel_with_reason(&this.global_this, reason); - } + // The abort beat the HTTP thread to the body, so the ReadableStream was + // never wired into a sink. Cancel it directly. + if !self.is_waiting_request_stream_start { + return; + } + if let HTTPRequestBody::ReadableStream(stream_ref) = &self.request_body { + self.is_waiting_request_stream_start = false; + if let Some(stream) = stream_ref.get(&self.global_this) { + stream.cancel_with_reason(&self.global_this, reason); } } } @@ -2266,15 +2299,107 @@ impl FetchTasklet { } } + /// Link `timer` into the VM timer heap `ms` from now. `ms` is never 0 here — + /// `fetch.rs` maps a 0/`false` deadline to "no deadline" and skips the call. + /// + /// # Safety + /// JS thread; `timer` is one of the tasklet's own `init_paused` nodes, not + /// currently linked into the heap. + unsafe fn arm_timeout(vm: *mut VirtualMachine, timer: *mut EventLoopTimer, ms: u32) { + // SAFETY: per fn contract; `timer_insert` links the node. + unsafe { + (*timer).next = bun_core::Timespec::ms_from_now( + bun_core::TimespecMockMode::AllowMockedTime, + i64::from(ms), + ); + VirtualMachine::timer_insert(vm, timer); + } + } + + /// Unlink `timer` from the VM timer heap. Idempotent. + /// + /// # Safety + /// JS thread; `timer` is one of the tasklet's own nodes and still live. + unsafe fn cancel_timeout(vm: *mut VirtualMachine, timer: *mut EventLoopTimer) { + // SAFETY: per fn contract; state == ACTIVE ⇒ linked into the per-VM heap. + unsafe { + if (*timer).state != TimerState::ACTIVE { + return; + } + VirtualMachine::timer_remove(vm, timer); + (*timer).state = TimerState::CANCELLED; + } + } + + /// Drop both deadlines. The only thing standing between a freed `FetchTasklet` + /// and a dangling heap node, so `clear_data()` must always reach it. + fn cancel_timeouts(&mut self) { + let vm = std::ptr::from_ref(self.javascript_vm).cast_mut(); + // SAFETY: JS thread; both nodes are our own and live while `self` is. + unsafe { + Self::cancel_timeout(vm, &raw mut self.connect_timeout_timer); + Self::cancel_timeout(vm, &raw mut self.total_timeout_timer); + } + } + + /// Abort only a request still dialling; once the transport is up the socket's + /// idle timer owns the deadline. One-shot, and `Signals.connected` is never + /// cleared, so a redirect that reopens a connection is not re-bounded. + pub(crate) fn on_connect_timeout(&mut self) { + self.connect_timeout_timer.state = TimerState::FIRED; + if self.signal_store.connected.load(Ordering::Acquire) { + return; + } + self.abort_with_timeout(jsc::CommonAbortReason::ConnectionTimeout); + } + + /// The whole-request `timeout` expired. Unlike the other two this fires in + /// any phase, including mid-body: it is a wall-clock bound on the whole + /// request, so a server trickling bytes forever still gets cut off. + pub(crate) fn on_total_timeout(&mut self) { + self.total_timeout_timer.state = TimerState::FIRED; + self.abort_with_timeout(jsc::CommonAbortReason::Timeout); + } + + /// Tear the request down with `reason`, the same way an `AbortSignal` would. + fn abort_with_timeout(&mut self, reason: jsc::CommonAbortReason) { + if self.signal_store.aborted.load(Ordering::Relaxed) { + return; + } + let reason = reason.to_js(&self.global_this); + reason.ensure_still_alive(); + self.abort_reason.set(&self.global_this, reason); + self.abort_task(); + self.cancel_request_body(reason); + } + pub(crate) fn queue( global: &JSGlobalObject, fetch_options: FetchOptions, promise: jsc::JSPromiseStrong, ) -> Result<*mut FetchTasklet, BunError> { http::http_thread::init(&http::http_thread::InitOpts::default()); + let connect_timeout_ms = fetch_options.connect_timeout_ms; + let total_timeout_ms = fetch_options.total_timeout_ms; let node = Self::get(global, fetch_options, promise)?; let node_ref = Self::from_raw_mut(node); + // Armed before the request reaches the HTTP thread, so time spent queued + // behind `BUN_CONFIG_MAX_HTTP_REQUESTS` counts against both. + let vm = std::ptr::from_ref(node_ref.javascript_vm).cast_mut(); + // SAFETY: JS thread; fresh `init_paused` nodes, never linked until now. + unsafe { + if connect_timeout_ms > 0 { + Self::arm_timeout( + vm, + &raw mut node_ref.connect_timeout_timer, + connect_timeout_ms, + ); + } + if total_timeout_ms > 0 { + Self::arm_timeout(vm, &raw mut node_ref.total_timeout_timer, total_timeout_ms); + } + } let mut batch = bun_threading::thread_pool::Batch::default(); node_ref.http.as_mut().unwrap().schedule(&mut batch); node_ref.poll_ref.ref_(bun_io::js_vm_ctx()); @@ -2521,7 +2646,12 @@ pub struct FetchOptions { pub headers: Headers, pub body: HTTPRequestBody, pub disable_timeout: bool, - /// Per-request idle-timeout override, from `fetch(url, { timeout: })`. + /// `connectTimeout` in milliseconds; 0 means no connect deadline. + pub connect_timeout_ms: u32, + /// The whole-request `timeout` in milliseconds; 0 means no overall deadline. + pub total_timeout_ms: u32, + /// `socketTimeout` as already-normalised seconds; `None` defers to the + /// process-wide `BUN_CONFIG_HTTP_IDLE_TIMEOUT` (300s). pub idle_timeout_seconds: Option, pub disable_keepalive: bool, pub disable_decompression: bool, @@ -2559,6 +2689,8 @@ impl Default for FetchOptions { headers: Headers::default(), body: HTTPRequestBody::default(), disable_timeout: false, + connect_timeout_ms: 0, + total_timeout_ms: 0, idle_timeout_seconds: None, disable_keepalive: false, disable_decompression: false, diff --git a/test/js/web/fetch/fetch-http2-client.test.ts b/test/js/web/fetch/fetch-http2-client.test.ts index 0cdd03adc9fa..159e2f0049b3 100644 --- a/test/js/web/fetch/fetch-http2-client.test.ts +++ b/test/js/web/fetch/fetch-http2-client.test.ts @@ -2020,15 +2020,17 @@ test("h2: per-request `timeout` extends the session idle deadline, and {timeout: `, ), // Global idle default = 20s. `{timeout:false}` contributes 0 to the - // session max and the `{timeout:1000}` sibling contributes 1s; the - // session must floor at the 20s global default so the no-timeout - // stream is not killed by the sibling's short explicit deadline. + // session max and the `{socketTimeout:1000}` sibling contributes 1s; the + // session must floor at the 20s global default so the no-timeout stream + // is not killed by the sibling's short explicit idle deadline. (Using + // `socketTimeout`, not `timeout` — `timeout` arms a per-request wall-clock + // deadline that is independent of the shared socket timer under test.) run( "20", /* js */ ` const [noTimeout, shortTimeout] = await Promise.all([ get({ timeout: false }), - get({ timeout: 1000 }), + get({ socketTimeout: 1000 }), ]); console.log(JSON.stringify({ noTimeout, shortTimeout })); `, @@ -2036,13 +2038,13 @@ test("h2: per-request `timeout` extends the session idle deadline, and {timeout: // Global idle default = 0 (disabled). A plain fetch with no `timeout` // option inherits effective deadline 0 without setting the // `disable_timeout` flag; the session must still disarm rather than - // letting the `{timeout:1000}` sibling arm the shared socket. + // letting the `{socketTimeout:1000}` sibling arm the shared socket. run( "0", /* js */ ` const [plain, shortTimeout] = await Promise.all([ get(undefined), - get({ timeout: 1000 }), + get({ socketTimeout: 1000 }), ]); console.log(JSON.stringify({ plain, shortTimeout })); `, diff --git a/test/js/web/fetch/fetch-timeout-options.test.ts b/test/js/web/fetch/fetch-timeout-options.test.ts new file mode 100644 index 000000000000..fb7ad9c0f399 --- /dev/null +++ b/test/js/web/fetch/fetch-timeout-options.test.ts @@ -0,0 +1,327 @@ +import { afterEach, expect, test } from "bun:test"; +import { bunEnv, bunExe } from "harness"; +import * as net from "node:net"; + +// Several tests below carry an explicit 30s budget rather than bun:test's 5s +// default. The socket timer is driven by uSockets' sweep, whose tick is 4 +// seconds (`LIBUS_TIMEOUT_GRANULARITY`), so the workload cannot be shrunk below +// it: proving the timer did *not* fire means out-waiting a full tick. Same +// reason `test/cli/install/bun-install-stalled-tls.test.ts` does it. + +// A raw TCP listener that accepts the connection, swallows the ClientHello, and +// never writes a byte back. To an `https:` client the socket is ESTABLISHED but +// the handshake stalls forever, so the request never leaves the connect phase. +// Same code path a dropped SYN takes (`first_call` is never reached), but +// deterministic enough to assert on. +const cleanup: Array<() => Promise | void> = []; +afterEach(async () => { + for (const fn of cleanup.splice(0)) await fn(); +}); + +async function stalledTlsPort(): Promise { + const sockets = new Set(); + const server = net.createServer(socket => { + sockets.add(socket); + socket.on("data", () => {}); + socket.on("close", () => sockets.delete(socket)); + socket.on("error", () => {}); + }); + await new Promise(resolve => server.listen(0, "127.0.0.1", resolve)); + cleanup.push(async () => { + for (const socket of sockets) socket.destroy(); + await new Promise(resolve => server.close(() => resolve())); + }); + return (server.address() as net.AddressInfo).port; +} + +test("connectTimeout rejects a stalled connect with TimeoutError", async () => { + const port = await stalledTlsPort(); + await expect( + fetch(`https://127.0.0.1:${port}/`, { connectTimeout: 250, tls: { rejectUnauthorized: false } }), + ).rejects.toMatchObject({ + name: "TimeoutError", + message: "The connection timed out.", + }); +}); + +test("timeout is a whole-request deadline and still bounds the connect phase", async () => { + const port = await stalledTlsPort(); + // `timeout` bounds the whole request, so a connect that never completes is + // caught by the overall deadline rather than a connect-specific one. + await expect( + fetch(`https://127.0.0.1:${port}/`, { timeout: 250, tls: { rejectUnauthorized: false } }), + ).rejects.toMatchObject({ + name: "TimeoutError", + message: "The operation timed out.", + }); +}); + +test("timeout fires mid-body, even while bytes are actively arriving", async () => { + // The whole point of `timeout`: the connection is never idle, so `socketTimeout` + // (10 minutes here) provably cannot be what cuts this off. The drip is bounded so + // a build without `timeout` fails the assertion rather than hanging. + const TIMEOUT_MS = 1_000; + let remaining = 40; // 40 * 50ms = ~2s of steady dripping, well past TIMEOUT_MS. + using server = Bun.serve({ + port: 0, + fetch() { + return new Response( + new ReadableStream({ + async pull(controller) { + if (remaining-- <= 0) { + controller.close(); + return; + } + controller.enqueue(new TextEncoder().encode("drip")); + await Bun.sleep(50); + }, + }), + ); + }, + }); + + const response = await fetch(server.url, { timeout: TIMEOUT_MS, socketTimeout: 600_000 }); + await expect(response.text()).rejects.toMatchObject({ + name: "TimeoutError", + message: "The operation timed out.", + }); +}); + +test("timeout does not fire on a request that finishes in time", async () => { + using server = Bun.serve({ port: 0, fetch: () => new Response("ok") }); + const response = await fetch(server.url, { timeout: 60_000 }); + expect(await response.text()).toBe("ok"); +}); + +test("connectTimeout wins over a longer timeout, with the more specific message", async () => { + // Both deadlines cover a stalled connect. The connect-specific one fires first + // and reports the reason that actually tells a retry policy what went wrong. + const port = await stalledTlsPort(); + await expect( + fetch(`https://127.0.0.1:${port}/`, { + connectTimeout: 250, + timeout: 60_000, + tls: { rejectUnauthorized: false }, + }), + ).rejects.toMatchObject({ + name: "TimeoutError", + message: "The connection timed out.", + }); +}); + +test("timeout of 0 disables every timeout", async () => { + using server = Bun.serve({ port: 0, fetch: () => new Response("ok") }); + const response = await fetch(server.url, { timeout: 0 }); + expect(await response.text()).toBe("ok"); +}); + +test("connectTimeout cancels a streaming request body with the timeout reason", async () => { + // https://fetch.spec.whatwg.org/#abort-fetch step 5: the request body must be + // cancelled with the abort reason, the same way `AbortSignal.timeout` does it. + const port = await stalledTlsPort(); + const { promise: cancelled, resolve: onCancel } = Promise.withResolvers<{ name: string; message: string }>(); + const body = new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode("chunk")); + }, + cancel(reason) { + onCancel(reason); + }, + }); + + await expect( + fetch(`https://127.0.0.1:${port}/`, { + method: "POST", + body, + connectTimeout: 250, + tls: { rejectUnauthorized: false }, + }), + ).rejects.toMatchObject({ name: "TimeoutError", message: "The connection timed out." }); + + const reason = await cancelled; + expect({ name: reason.name, message: reason.message }).toEqual({ + name: "TimeoutError", + message: "The connection timed out.", + }); +}); + +test("without connectTimeout, a stalled connect falls through to the socket timer", async () => { + // Fail-safe for the above: prove the rejection comes from the connect + // deadline and not from something else in the stalled-handshake path. With no + // connect deadline the idle timer is what eventually fires, and it reports a + // different message. + const port = await stalledTlsPort(); + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `try { + await fetch("https://127.0.0.1:${port}/", { tls: { rejectUnauthorized: false } }); + } catch (e) { + console.log(e.name + ": " + e.message); + }`, + ], + env: { ...bunEnv, BUN_CONFIG_HTTP_IDLE_TIMEOUT: "1" }, + stderr: "pipe", + }); + const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout: stdout.trim(), exitCode }).toEqual({ + stdout: "TimeoutError: The operation timed out.", + exitCode: 0, + }); +}, 30_000); + +test("connectTimeout does not fire once connected", async () => { + // The server answers long after `connect` elapses, but the connect phase + // itself finished immediately: only the idle timer may apply past that point. + const { promise: requestStarted, resolve: onRequest } = Promise.withResolvers(); + const { promise: release, resolve: respond } = Promise.withResolvers(); + using server = Bun.serve({ + port: 0, + async fetch() { + onRequest(); + await release; + return new Response("late"); + }, + }); + + const pending = fetch(server.url, { connectTimeout: 50, socketTimeout: 60_000 }); + await requestStarted; + // The connect deadline has long expired by the time the server replies. + await Bun.sleep(200); + respond(); + expect(await (await pending).text()).toBe("late"); +}); + +test("socketTimeout rejects a stalled response body", async () => { + const { promise: release, resolve: finish } = Promise.withResolvers(); + using server = Bun.serve({ + port: 0, + fetch() { + return new Response( + new ReadableStream({ + async start(controller) { + controller.enqueue(new TextEncoder().encode("first")); + await release; + controller.close(); + }, + }), + ); + }, + }); + + const response = await fetch(server.url, { socketTimeout: 1_000 }); + await expect(response.text()).rejects.toMatchObject({ name: "TimeoutError" }); + finish(); +}, 30_000); + +// Note: that a per-request `socketTimeout` overrides the process-wide +// `BUN_CONFIG_HTTP_IDLE_TIMEOUT` is already covered by "socketTimeout rejects a +// stalled response body" above: it fires at ~1s against the 300s process +// default, which only happens if the per-request value replaced the default. +// The reverse direction (a larger per-request value outlasting a smaller +// default) exercises the same `effective_idle_timeout_seconds` branch, so it is +// not tested separately. + +// https://github.com/oven-sh/bun/issues/16682 +test("a numeric timeout longer than the socket-idle default is respected", async () => { + const { promise: release, resolve: respond } = Promise.withResolvers(); + using server = Bun.serve({ + port: 0, + async fetch() { + await release; + return new Response("ok"); + }, + }); + // 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" }, + stderr: "pipe", + }); + await Bun.sleep(6_000); + respond(); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout: stdout.trim(), exitCode }).toEqual({ stdout: "ok", exitCode: 0 }); + expect(stderr).not.toContain("TimeoutError"); +}, 30_000); + +test("timeout: false disables the socket timer", async () => { + const { promise: release, resolve: respond } = Promise.withResolvers(); + using server = Bun.serve({ + port: 0, + async fetch() { + await release; + return new Response("ok"); + }, + }); + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `const r = await fetch(${JSON.stringify(server.url.href)}, { timeout: false }); + console.log(await r.text());`, + ], + env: { ...bunEnv, BUN_CONFIG_HTTP_IDLE_TIMEOUT: "1" }, + stderr: "pipe", + }); + await Bun.sleep(6_000); + respond(); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout: stdout.trim(), exitCode }).toEqual({ stdout: "ok", exitCode: 0 }); + expect(stderr).not.toContain("TimeoutError"); +}, 30_000); + +test("connectTimeout of 0 means no connect deadline", async () => { + using server = Bun.serve({ port: 0, fetch: () => new Response("ok") }); + const response = await fetch(server.url, { connectTimeout: 0 }); + expect(await response.text()).toBe("ok"); +}); + +test("an explicit socketTimeout outranks timeout: false", async () => { + // `timeout: false` turns everything off, but a socketTimeout the caller spelled + // out is what they actually meant, so it has to survive. + const { promise: release, resolve: respond } = Promise.withResolvers(); + using server = Bun.serve({ + port: 0, + fetch() { + return new Response( + new ReadableStream({ + async start(controller) { + controller.enqueue(new TextEncoder().encode("first")); + await release; + controller.close(); + }, + }), + ); + }, + }); + + const response = await fetch(server.url, { timeout: false, socketTimeout: 1_000 }); + await expect(response.text()).rejects.toMatchObject({ name: "TimeoutError" }); + respond(); +}, 30_000); + +// Argument validation throws synchronously, like `maxRedirects` and `protocol`. +test.each([ + [-1, "fetch: 'timeout' must be a non-negative integer number of milliseconds, or false"], + [1.5, "fetch: 'timeout' must be a non-negative integer number of milliseconds, or false"], + [NaN, "fetch: 'timeout' must be a non-negative integer number of milliseconds, or false"], + ["5s", "fetch: 'timeout' must be a boolean or a non-negative integer number of milliseconds"], + [{ total: 5 }, "fetch: 'timeout' must be a boolean or a non-negative integer number of milliseconds"], +])("rejects an invalid timeout: %p", (value, message) => { + expect(() => fetch("http://127.0.0.1:1/", { timeout: value as never })).toThrow(message); +}); + +test.each([ + ["connectTimeout", "fetch: 'connectTimeout' must be a non-negative integer number of milliseconds, or false"], + ["socketTimeout", "fetch: 'socketTimeout' must be a non-negative integer number of milliseconds, or false"], +])("rejects an invalid %s", (key, message) => { + expect(() => fetch("http://127.0.0.1:1/", { [key]: -5 } as never)).toThrow(message); +}); From 3baeef9795118dee993c4c8c76eddadb9bf525bb Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 9 Jul 2026 18:49:13 +0000 Subject: [PATCH 2/2] review: revert rearm_timeout doc comment to main's; drop two dead stderr 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. --- src/http/h2_client/ClientSession.rs | 11 ++++++----- test/js/web/fetch/fetch-timeout-options.test.ts | 6 ++---- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/http/h2_client/ClientSession.rs b/src/http/h2_client/ClientSession.rs index 976b7d3bc62b..5da3d6ea057a 100644 --- a/src/http/h2_client/ClientSession.rs +++ b/src/http/h2_client/ClientSession.rs @@ -516,11 +516,12 @@ impl ClientSession { } } - /// Re-arm the shared socket's idle timer at the longest any attached client - /// asked for — treating `0` ("disarm") as unbounded, not as a minimum. The - /// fire handler kills the whole session, so a shorter value would cut off a - /// longer-timeout or `timeout: false` sibling; this only delays the short - /// one. Per-stream idle tracking would be the exact answer. + /// Re-arm the shared socket's idle timer based on the aggregate of every + /// attached client. With multiplexed streams the per-request + /// `disable_timeout` flag can't drive the socket directly (last writer + /// would win and a `{timeout:false}` long-poll could be killed by a + /// sibling re-arming, or strip the safety net from one that wants it), + /// so the session disarms only when *every* attached client opted out. fn rearm_timeout(&mut self) { // The socket is shared by every stream on the session, so arm the // longest effective idle timeout among them (0 = every client's diff --git a/test/js/web/fetch/fetch-timeout-options.test.ts b/test/js/web/fetch/fetch-timeout-options.test.ts index fb7ad9c0f399..1c33749bccc2 100644 --- a/test/js/web/fetch/fetch-timeout-options.test.ts +++ b/test/js/web/fetch/fetch-timeout-options.test.ts @@ -247,9 +247,8 @@ test("a numeric timeout longer than the socket-idle default is respected", async }); await Bun.sleep(6_000); respond(); - const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); expect({ stdout: stdout.trim(), exitCode }).toEqual({ stdout: "ok", exitCode: 0 }); - expect(stderr).not.toContain("TimeoutError"); }, 30_000); test("timeout: false disables the socket timer", async () => { @@ -273,9 +272,8 @@ test("timeout: false disables the socket timer", async () => { }); await Bun.sleep(6_000); respond(); - const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); expect({ stdout: stdout.trim(), exitCode }).toEqual({ stdout: "ok", exitCode: 0 }); - expect(stderr).not.toContain("TimeoutError"); }, 30_000); test("connectTimeout of 0 means no connect deadline", async () => {