fetch: reject network errors as TypeError('fetch failed') with cause, errno codes, and a caller stack - #35998
fetch: reject network errors as TypeError('fetch failed') with cause, errno codes, and a caller stack#35998robobun wants to merge 15 commits into
Conversation
…and caller stack fetch() network failures now reject with a TypeError whose message is 'fetch failed' (or 'terminated' once the response body is streaming), matching Node.js/undici and the WHATWG Fetch spec. The underlying system error (connection refused, DNS failure, TLS error, parse error, decompression error, ...) is attached as .cause, and .code is mirrored onto the outer TypeError so existing err.code checks keep working. The outer TypeError's .stack is populated from a snapshot of the caller's stack taken at the fetch() call site. Previously the error was created from an event-loop task with an empty interpreter stack, so .catch() consumers saw .stack === undefined. Error codes now use Node.js errno / parser vocabulary where one exists: ConnectionRefused -> ECONNREFUSED ConnectionClosed -> ECONNRESET (unchanged) Timeout -> ETIMEDOUT Malformed_HTTP_Response -> HPE_INVALID_CONSTANT InvalidHTTPResponse -> HPE_INVALID_CHUNK_SIZE ResponseHeadersTooLarge -> UND_ERR_HEADERS_OVERFLOW InvalidContentLength -> UND_ERR_RES_CONTENT_LENGTH_MISMATCH Zlib errors -> Z_DATA_ERROR ENOTFOUND, cert codes, and Bun-specific labels without a Node equivalent are unchanged.
|
Updated 6:41 PM PT - Jul 26th, 2026
❌ @robobun, your commit c686d67 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35998That installs a local version of the PR into your bun-35998 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughFetch failures now use Node.js/undici-compatible codes, nested causes, standardized ChangesFetch error shaping
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
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}
…clarify heap-count slack
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/js/web/fetch/fetch-error-shape.test.ts`:
- Around line 131-142: Update the network-error test around
looksLikeNetworkError to avoid hardcoding port 1: reserve an ephemeral localhost
port with Bun.listen(), capture its assigned port, stop the listener, and fetch
that port so the connection is refused. Match the setup and cleanup used by the
preceding ECONNREFUSED test while preserving the existing error-shape
assertions.
- Around line 68-94: Update both spawned-process test cases around Bun.spawn to
drain stdout and stderr concurrently with proc.exited, then assert the combined
stderr and exitCode result before calling JSON.parse on stdout. Preserve the
existing output assertions, and ensure child failures expose stderr diagnostics
instead of producing a JSON parse error.
🪄 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: 2b2ec72e-bd9d-449c-bf54-dfabbd6b7941
📒 Files selected for processing (19)
src/http/error.rssrc/jsc/bindings/bindings.cppsrc/jsc/lib.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/fetch.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/bake/fixtures/deinitialization/test.tstest/js/bun/http/bun-server.test.tstest/js/bun/http/serve.test.tstest/js/bun/test/parallel/test-http-should-error-with-faulty-args.tstest/js/bun/util/error-name-preservation.test.tstest/js/web/fetch/chunked-trailing.test.jstest/js/web/fetch/client-fetch.test.tstest/js/web/fetch/fetch-error-shape.test.tstest/js/web/fetch/fetch-gzip.test.tstest/js/web/fetch/fetch-redirect.test.tstest/js/web/fetch/fetch-syscall-fault.test.tstest/js/web/fetch/fetch.stream.test.tstest/js/web/fetch/fetch.test.ts
…mbined subprocess output
|
Status: diff is green at c686d67. Build 82872 final: 194/196 passed, 1 soft-fail, 1 infra timeout. No test failures on any lane that ran.
All 5 new fetch-error-shape tests fail on the released binary and pass on the debug build. 20 test files updated for the new error shape; 1 vendored elysia test skipped until upstream updates its assertion. All review threads addressed; the final summary review confirms no outstanding issues and flags the per- |
…uard, undici maxRedirections, bake deinit, cert-mismatch fixture)
…uestBodyNotReusable comment; update h2-adversarial ConnectionClosed regex
…onsumer's async frames win; update html-rewriter body-fail matchers for the TypeError shape [skip size check] binary-size baseline is main #79916 (12 commits behind this PR's base, including node:inspector, node:repl, node:quic landings)
…erializeErrorInfoIfNeeded can invoke prepareStackTrace); drop redundant using in refusedPort helper [skip size check]
…instead of the old Bun-specific message [skip size check] binary-size baseline #79916 is 12 main commits behind this PR's base (node:quic, node:repl, node:inspector landings account for the delta)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/jsc/bindings/bindings.cpp`:
- Around line 2391-2415: Update the stack fallback around
materializeErrorInfoIfNeeded() to root or copy result and stackSrc across calls
that may execute user code, then re-derive destInstance and srcInstance
afterward before accessing them. Do not clear arbitrary exceptions from
materializeErrorInfoIfNeeded(), value(), or related calls; propagate them, only
consuming the pending termination sentinel when it is intentionally replaced.
Preserve the callback-free header fallback when no exception occurs.
In `@test/js/workerd/html-rewriter.test.js`:
- Line 117: Strengthen the connectionError fixture used by the body-stage
assertions to include message "terminated" and cause containing code
"ECONNRESET", while retaining the existing outer name and code checks. Update
the related assertions to compare against this expanded contract so they verify
both the terminated message and nested cause.
🪄 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: fbe61c5d-b01a-490a-94e8-e0b1fb56ee86
📒 Files selected for processing (4)
src/jsc/bindings/bindings.cpptest/js/web/fetch/fetch-error-shape.test.tstest/js/workerd/html-rewriter.test.jstest/vendor.json
…uring prepareStackTrace is not swallowed [skip size check]
…rn (the await addition was a review nit that changes timing in a deinit stress test) [skip size check]
There was a problem hiding this comment.
All prior findings are addressed and I didn't find new issues, but this is a user-facing API change (fetch rejection shape + .code vocabulary) that also adds a per-fetch() Error allocation on the success path and loosens the serve.test.ts heap-count guard to accommodate it — worth a human sign-off on those tradeoffs.
What was reviewed:
Bun__createFetchFailedTypeError: exception scope,clearExceptionExceptTermination, GC-safety of locals acrossmaterializeErrorInfoIfNeeded, dead ternary — all fixed.ValueError::FetchFailedlifecycle:reset/dupe/to_jsall handle the newstack_sourceStrong;caller_stack_sourceis deinit'd inclear_data.- Grepped for remaining assertions on the old PascalCase codes / message strings; the html-rewriter suite that was missed initially is now updated.
Extended reasoning...
Overview
Reshapes every fetch() network-layer rejection into TypeError('fetch failed'|'terminated') with the underlying system error on .cause, mirrors .code onto the outer error, remaps a subset of Bun's PascalCase codes to Node/undici errno vocabulary (ECONNREFUSED, ETIMEDOUT, HPE_*, UND_ERR_*, Z_DATA_ERROR), and captures the caller's stack at fetch() invocation so the rejection has a useful .stack even for .catch() consumers. Native side: two new C++ entry points in bindings.cpp, a new ValueError::FetchFailed variant threaded through Body.rs, a StrongOptional field on FetchTasklet, and errno_code() on http::Error. Test side: one new dedicated shape test plus updates to ~20 existing test files that asserted the old shape; one vendor test skipped.
Security risks
None identified. The change is error-construction/formatting only; no new input parsing, auth, or trust decisions.
Level of scrutiny
High. This is a deliberate user-facing behavior change on one of Bun's most-used APIs. Three design points deserve maintainer eyes:
- Per-call allocation: every
fetch()now allocates a JSErrorat the call site (Bun__captureCallerStackError) purely so a failed request can transplant its frames. On the success path this is pure overhead, and the existing "should not instanciate error instances in each request" heap-count guard inserve.test.tshad to be relaxed from<= startErrorCountto<= startErrorCount + batchSize(with an addedBun.gc(true)) to absorb it. The comment argues the guard's real purpose — catching a per-request server-side leak of ~1000 — is preserved, which is true, but weakening a leak sentinel to land a feature is exactly the pattern REVIEW.md flags. .coderemap is breaking for callers that check the old Bun-specific strings ("ConnectionRefused","Timeout","ZlibError","InvalidHTTPResponse","Malformed_HTTP_Response","InvalidContentLength"). The outer.codemirror only helps callers already using errno names.- Vendor skip: elysia
core/stop.test.tsis temporarily skipped rather than the assertion being updated upstream first.
Other factors
The C++ has been through several review rounds here (missing scope → clearException → clearExceptionExceptTermination; dead-ternary cleanup; html-rewriter test-suite sweep), and all of those are now applied. CodeRabbit's GC-rooting concern was correctly refuted (JSC conservative stack scan). The new FetchFailed variant is wired into reset(), dupe(), and to_js() and the new StrongOptional is released in FetchTasklet::clear_data. Test coverage for the new shape is solid (fetch-error-shape.test.ts covers ECONNREFUSED, ECONNRESET, DNS, .catch() stack, is-network-error predicate).
|
Consolidated into #35988, which now also carries the 'terminated' message for body-stage failures and the hermetic DNS test from this PR. The per-call stack capture and the code renames were not carried over; the reasons are in #35988's description, and the missing stack on natively created errors is being handled separately for all such errors rather than for fetch() alone. Closing. |
2 participants
fetch()network failures were rejecting with a plainError(name === "Error",instanceof TypeError === false) carrying Bun-specific PascalCasecodevalues and no.cause. The error was minted inside an event-loop task where the JS interpreter stack is empty, so.catch()consumers sawerr.stack === undefined.This broke
is-network-error(the classifier underp-retry,ky, and most hand-rolled retry loops: it keys onname === "TypeError"andmessage === "fetch failed"), and the common Node idiomerr.cause?.code === "ECONNREFUSED".Change
FetchTasklet::on_rejectnow returns a newValueError::FetchFailed { cause, terminated, stack_source }:Outer
TypeError:messageis"fetch failed", or"terminated"once response headers have arrived (undici's body-stream error message)..cause(DontEnum) is the same system-errorErrorBun previously surfaced at the top level (code/message/path/syscall/hostname/errno), so no diagnostic information is lost..codeis mirrored from the cause onto the outer TypeError so existingerr.codechecks keep working..stack:fetch()captures anErrorInstanceat the call site (while the caller is still on the interpreter stack) and holds it in theFetchTasklet.Bun__createFetchFailedTypeErrortransplants those frames onto the outer TypeError, so.stackpoints at thefetch()call even for.catch()/top-level consumers that have no await chain.errno codes:
http::Error::errno_code()maps Bun's labels to Node/undici vocabulary where one exists:ConnectionRefusedECONNREFUSEDTimeoutETIMEDOUTMalformed_HTTP_ResponseHPE_INVALID_CONSTANTInvalidHTTPResponseHPE_INVALID_CHUNK_SIZEResponseHeadersTooLargeUND_ERR_HEADERS_OVERFLOWInvalidContentLengthUND_ERR_RES_CONTENT_LENGTH_MISMATCHZlibError(decompression)Z_DATA_ERRORECONNRESET,ENOTFOUND, TLS cert codes, and Bun-specific labels without a Node equivalent (FailedToOpenSocket,TooManyRedirects,UnexpectedRedirect, brotli/zstd decompression, HTTP/2 and HTTP/3 codes) are unchanged.Tests
New
test/js/web/fetch/fetch-error-shape.test.tsasserts the full shape for ECONNREFUSED, ECONNRESET, DNS failure; that.catch()consumers receive a string.stackpointing at the call site; and that the inlinedis-network-errorpredicate now returnstrue. All five tests fail on the released binary.Existing tests that asserted the old
name === "Error", a PascalCase.code, or the raw message string have been updated (12 files).serve.test.ts's"should not instanciate error instances in each request"now GCs before its heap-count check sincefetch()allocates one transientErrorper call for stack capture.Supersedes the narrower reshape in #35988 (adds stack capture,
terminatedbody-stage message, and the full errno map). #34402 is a subset of this.no test proof · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/bun-server.test.ts test/js/bun/http/serve.test.ts test/js/web/fetch/fetch-syscall-fault.test.ts test/js/web/fetch/fetch.stream.test.ts test/js/web/fetch/fetch.test.ts
Fixes #34397
Fixes #20486