Repository navigation
fix(util): lift undici's 300s timeouts on Node - #54793
Yedigaryan wants to merge 3 commits into
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix works on Node. Against a local server that holds headers for 310s and then waits 310s before the last chunk, the base's built-in fetch and Effect FetchHttpClient fail at 301s with UND_ERR_HEADERS_TIMEOUT. With this PR, both UndiciFetch.create() and the HttpClient layer get the full body at 620s. undici-fetch.test.ts passes 5/5 and packages/core type-checks cleanly. I didn't exercise the aisdk.ts path end to end. Bun's fetch finished a 310s header delay with or without timeout: false, so the Bun-compiled CLI and Desktop don't hit this limit and won't change. The fix matters for the Node build and embedded Node use.
The public API is unchanged. The only addition is the undici dependency in packages/core (undici is already in the tree through @effect/platform-node).
Main concern: core is also bundled for workerd (see packages/server/script/workerd-probe.ts). With this change, workerd loads undici and creates an Agent as soon as app-node-platform.ts loads, and it would send provider requests through undici instead of the platform fetch. Details are inline.
| // turned off. The Agent only works with undici's own fetch: Node's bundled undici rejects an undici@8 Agent | ||
| // (UND_ERR_INVALID_ARG). Bun's fetch has no such defaults, so it keeps its own. | ||
| export function create(runtime: { readonly bun?: string } = process.versions): Fetch | undefined { | ||
| if (runtime.bun) return undefined |
There was a problem hiding this comment.
This treats every runtime that isn't Bun as Node, so workerd (core is bundled for it under the workerd condition, see packages/server/script/workerd-probe.ts) also gets an undici Agent and undici's socket-based fetch for provider requests, instead of the platform fetch. The static import ... from "undici" also puts undici into the workerd graph; with this PR the probe bundle grows from 17.2 to 18.2 MiB. packages/util/src/effect/app-node-platform.ts uses deep imports specifically to keep undici out of workerd. Could this use a node import condition, the way #runtime-import does, so Bun and workerd get the plain global fetch and undici is only loaded on Node?
| service: RequestExecutor.Service, | ||
| layer: RequestExecutor.layer, | ||
| // Provider requests get their own client: Node's fetch would cut slow responses at undici's 300s defaults. | ||
| layer: RequestExecutor.layer.pipe(Layer.provide(UndiciFetch.layer())), |
There was a problem hiding this comment.
UndiciFetch.layer() evaluates its default argument shared() here, when the module loads, not when the layer is built. So importing this module (and so anything reaching llmClient) loads undici and creates the Agent right away on every runtime that isn't Bun, workerd included. Creating it lazily inside Layer.effect would keep that cost (and any runtime incompatibility) off the import path.
There was a problem hiding this comment.
The new commits fix both earlier points. undici is now loaded only under the node condition, so the workerd bundle has no undici code (the workerd probe and server/test/workerd.test.ts pass). The fetch is now chosen when the layer is built, not when the module loads. I re-ran the Node check with the node condition bundle against a server that holds headers for 310s, then waits 310s before the last chunk. The built-in fetch still fails at 300s with UND_ERR_HEADERS_TIMEOUT, and both UndiciFetch.shared() and the HttpClient layer get the full body at 620s. undici-fetch.test.ts passes 5/5 and packages/core type-checks cleanly. The public API is unchanged. No new concerns.
|
Thanks, both points were right. Fixed in 295390b:
Rebased onto current |
|
Hi @rekram1-node, friendly nudge on this one when you have a moment. It fixes #26602, where Node's built-in fetch cuts off provider requests after undici's 300s It sends provider requests through undici's own fetch with those two timeouts lifted, only on Node. Bun is unaffected, since #55007 already covers it, and workerd is unaffected because undici is behind an import condition and is not in its bundle. The bot review's workerd concern is addressed in 295390b, and the PR is rebased and green. Happy to adjust the approach if you would rather solve this differently. |
On Node, provider requests go through the built-in fetch, which enforces undici's default 300s headersTimeout and bodyTimeout. A slow local model or an SSE stream with a long gap is cut at about 301s with UND_ERR_HEADERS_TIMEOUT, whatever headerTimeout, chunkTimeout or timeout the user configured. Send provider requests through undici's own fetch with an Agent whose socket timeouts are off, since the limits above this layer already enforce the configured ones. Both paths are covered: the AI SDK fetch in aisdk.ts and the native request executor's HttpClient. Bun is unchanged, and a custom fetch or a replaced HttpClient stays in charge. The Agent has to be paired with undici's fetch: Node 22's built-in fetch rejects an undici@8 Agent with UND_ERR_INVALID_ARG. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
The workerd bundle was pulling in undici and creating an Agent on module load. Resolve the undici fetch through a #undici-fetch import condition so Bun and workerd keep their own fetch, and pick the fetch when the layer is built rather than at import. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
dev now routes provider, MCP and HttpClient requests through one httpFetch node that defaults to the runtime fetch. Move the undici fetch into util and make fetchLayer() use it on Node, so every outbound path loses undici's 300s timeouts. An embedder-supplied fetch still wins; Bun and workerd are unchanged. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
Yedigaryan
changed the title
|
Update: after #61582 landed on This supersedes the layout in my earlier reply ( |
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
Issue for this PR
Closes #26602
Type of change
What does this PR do?
On Node, provider requests go through the built-in fetch, which enforces undici's default 300s
headersTimeoutandbodyTimeout. A slow local model, or an SSE stream with a long gap between chunks, is cut at about 301s withUND_ERR_HEADERS_TIMEOUT, whateverheaderTimeout,chunkTimeoutortimeoutthe user configured.V2 already enforces those configured limits itself, so the socket-level 300s limits are a hidden second timer. This turns them off by sending requests through undici's own
fetchwith anAgentset toheadersTimeout: 0, bodyTimeout: 0.devnow routes AI SDK providers, remote MCP servers and the native request executor through onehttpFetchnode (#61582). So the fix lives there instead of at each call site:packages/util/src/effect/app-node-platform.ts:fetchLayer()uses undici's fetch on Node. An embedder-supplied fetch still wins, and with neither, theFetchHttpClient.Fetchin scope is kept as before.#undici-fetchimport condition inpackages/util(nodeloads undici,bun,workerdand the default load a no-op). Bun and workerd keep their own fetch.The
Agenthas to be paired with undici'sfetch. Passing an undici@8Agentasdispatcherto Node 22's built-in fetch fails withUND_ERR_INVALID_ARG, so that approach doesn't work.This replaces #50656, which targeted V1 and was closed when
devmoved to V2.How did you verify your code works?
packages/util/src/undici-fetch.test.ts(5 pass).bun typecheckis clean inpackages/util,packages/coreandpackages/server; the fullpackages/util(145) andpackages/server(122) suites pass, andpackages/core/test/aisdk.test.tspasses (46).nodecondition and run on Node 22: the fetch fromfetchLayer()is notglobalThis.fetch, and a request through it succeeds. An earlier version of this fix was run on Node 22 against a local server that holds the headers back for 310s and then waits 310s before the last chunk: the built-in fetch fails withUND_ERR_HEADERS_TIMEOUTat 301s and the undici fetch succeeds at 620s with the full body. I did not repeat the 620s run after moving the code toutil.bun run probe:workerdinpackages/server) still builds, and its output contains no undici.bun testcan't reproduce the 300s timeout, because Bun shimsundiciand has no such default, so the unit tests cover the selection logic and the Node run covers the behavior.Screenshots / recordings
N/A
Checklist
🤖 Generated with Claude Code