Conversation
WalkthroughFetch network failures now use ChangesFetch network error shape alignment
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:16 PM PT - Aug 23rd, 2026
❌ @robobun, your commit 54f1f27 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35988That installs a local version of the PR into your bun-35988 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Regarding the duplicate-PR flags:
Happy to close #34402 in favour of this once reviewed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.test.ts`:
- Around line 2082-2086: Await each asynchronous rejection assertion so test
failures are propagated: update the fetch redirect matcher in
test/js/web/fetch/fetch.test.ts:2082-2086, the ECONNRESET matcher in
test/bake/fixtures/deinitialization/test.ts:61, the ECONNREFUSED matcher in
test/bake/fixtures/deinitialization/test.ts:71, and the res.text() rejection
matcher in test/js/bun/http/serve.test.ts:2679. Return or await each matcher
without changing its expectations.
🪄 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: 4b280b01-629f-47aa-9d6c-aa9f51258b3e
📒 Files selected for processing (14)
src/jsc/SystemError.rssrc/jsc/bindings/bindings.cppsrc/jsc/bindings/headers.hsrc/runtime/webcore/Body.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/bake/fixtures/deinitialization/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/client-fetch.test.tstest/js/web/fetch/fetch-network-error.test.tstest/js/web/fetch/fetch.stream.test.tstest/js/web/fetch/fetch.test.tstest/js/workerd/html-rewriter.test.js
There was a problem hiding this comment.
All prior feedback addressed — the five follow-up commits (646e165 through d9d67e9) cover every inline comment I left. No new issues found this pass.
Deferring to a human for the API-shape decision: this changes the user-visible rejection contract of fetch() (Error→TypeError, descriptive message→"fetch failed", ConnectionRefused→ECONNREFUSED) and overlaps with #35855 / #34402.
What was reviewed
- New
SystemError__toFetchTypeErrorInstanceC++ binding — theclearException()pattern matches the siblingSystemError__toErrorInstanceimmediately above it. ValueError::FetchErrorvariant threaded throughreset/to_js/dupe— all three match arms updated.- Grepped remaining
ConnectionRefusedreferences intest/— the survivors are either comments, non-fetch (installer), or already includeECONNREFUSEDas an alternative in their regex.
Extended reasoning...
Overview
This PR reshapes every fetch() network-error rejection from a bare Error with a descriptive message into TypeError('fetch failed', {cause: <original Error>}), matching the Fetch spec (§4.1 step 12.3) and Node/undici. It adds a new ValueError::FetchError(SystemError) variant in Body.rs, a new C++ binding SystemError__toFetchTypeErrorInstance in bindings.cpp, routes FetchTasklet::on_reject through it, remaps ConnectionRefused→ECONNREFUSED (with syscall: 'connect'), and updates 12 test files plus adds a dedicated fetch-network-error.test.ts.
Security risks
None identified. This is error-object construction on the rejection path; no new input parsing, no auth/TLS logic changes. The .code mirroring onto the outer TypeError uses putDirect on a freshly-created error object, not user-controlled data.
Level of scrutiny
High — this is a user-visible behavioral change to a core Web API. Code that currently checks err.code === 'ConnectionRefused', err.constructor === Error, or matches on err.message will observe different values. The PR mitigates this by mirroring .code onto the outer error and preserving the descriptive message on .cause.message, but it is still a break for anyone reading the outer .message or the old ConnectionRefused code string. That trade-off (spec/Node compat + is-network-error classifier support vs. Bun-specific back-compat) is a maintainer call.
Other factors
- Prior review rounds: I left five rounds of inline findings (test-weakening, missed test updates, un-awaited
.rejects, harness conventions), all now addressed by commits 646e165, ca44353, d190934, 3bb2573, d9d67e9. All threads are resolved. - Overlapping PRs: #35855 and #34402 touch the same code paths; the author has explained the relationship in the thread, but which approach lands is a human decision.
- Native code: The new C++ function's
DECLARE_TOP_EXCEPTION_SCOPE+clearException()onBun::toJSfailures exactly mirrors the existingSystemError__toErrorInstancein the same file (lines ~2375–2418), so it follows local convention rather than introducing a new pattern. - Test coverage: The new
fetch-network-error.test.tscovers connection-refused, socket-closed-pre-headers, malformed-response, and body-truncated shapes, plus runs the inlinedis-network-errorpredicate against a real rejection. I re-greppedtest/for remainingConnectionRefusedstring matches — none should break (all either non-fetch, comments, or already regex-alternate onECONNREFUSED). - CI: Build #82742 for the latest commit was still running at review time.
|
Opened #35998 which covers this reshape plus caller-stack capture (so |
There was a problem hiding this comment.
The prior review feedback has all been addressed and I didn't find new issues, but this is a user-facing behavioral change to fetch()'s rejection shape (message/name/instanceof/code all change, a vendor test is skipped, and it overlaps with #35855/#34402), so it needs a maintainer to sign off on the API decisions rather than a bot approval.
What was reviewed:
SystemError__toFetchTypeErrorInstancefollows the sameDECLARE_TOP_EXCEPTION_SCOPE+clearExceptionpattern as the adjacentSystemError__toErrorInstance.ValueError::FetchErroris wired intoreset/to_js/dupealongsideSystemError, so no lifecycle path drops it.- All previously-flagged un-awaited
.rejectsmatchers and stale outer-message assertions (bun-server, undici, untrusted-cert, html-rewriter, client-fetch) are fixed in the current diff.
Extended reasoning...
Overview
This PR changes every fetch() network-error rejection from a plain Error with a descriptive message to TypeError('fetch failed', {cause: <original Error>}), matching the Fetch spec §4.1.12.3 and Node/undici. It touches FetchTasklet::on_reject (Rust), adds a ValueError::FetchError variant to Body.rs, adds a new C++ binding SystemError__toFetchTypeErrorInstance, remaps ConnectionRefused→ECONNREFUSED, and updates ~12 test files plus adds a new dedicated shape test and a vendor-test skip.
Security risks
None identified. The change reshapes error objects on the rejection path; it doesn't touch validation, TLS decisions, or any allow/deny gate. The new C++ reads only from the Rust-owned SystemError struct.
Level of scrutiny
High — this is a user-visible breaking change to the most-used Web API. Every existing user check like err.message.includes('Unable to connect'), err.code === 'ConnectionRefused', or err.name === 'Error' changes behavior. The PR itself had to update a dozen tests and skip an elysia vendor test, which is a direct signal of ecosystem impact. REVIEW.md's API-design section calls for maintainer agreement on user-facing surface changes; the specific choices here (mirroring .code onto the outer TypeError when Node does not, mapping only two errors to libuv errno names while keeping Bun names for the rest, the exact 'fetch failed' message) are design calls a human should ratify.
Other factors
- Overlapping PRs: #35855 and #34402 cover the same ground with different tradeoffs; a maintainer needs to coordinate which lands and close the others.
- Prior feedback addressed: the earlier 🔴 finding (three tests still asserting the old outer message) was fixed in d190934; the un-awaited
.rejectsmatchers were fixed in ca44353 and 3bb2573; the html-rewriter regex weakening was fixed in d9d67e9; the deinitialization test-precision nit was fixed. All inline threads are resolved. - C++ pattern: the new binding's
scope.clearException()usage matches the immediately-adjacentSystemError__toErrorInstancebyte-for-byte, so it's consistent with local convention for error-construction helpers. - Test coverage: the new
fetch-network-error.test.tsis thorough (connection-refused, pre-header close, malformed response, mid-body truncation, and the inlinedis-network-errorpredicate). - CodeRabbit's linked-issues check notes #20486 also covers invalid-URL/protocol TypeError shapes not addressed here; whether to keep that
Fixestag is a maintainer call.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-http2-client.test.ts`:
- Around line 855-858: Update the subprocess assertions in
test/js/web/fetch/fetch-http2-client.test.ts at lines 855-858, 1157-1160, and
1179-1182: retain concurrent draining of stdout, stderr, and process exit,
assert exitCode is 0, and include stderr alongside stdout in failure diagnostics
while preserving each case’s expected stdout.
🪄 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: 3740e85b-adac-4689-b403-d29a483c8309
📒 Files selected for processing (2)
test/js/web/fetch/fetch-http2-client.test.tstest/vendor.json
There was a problem hiding this comment.
All earlier review feedback has been addressed and no new issues surfaced this pass, but this is a user-facing change to fetch()'s error shape (outer .message, .code for ConnectionRefused, instanceof TypeError) that breaks existing consumers keying on the old shape — the elysia vendor skip is one example — so it needs a maintainer's call on the compatibility trade-off. Note also that #35998 was opened after this PR and is described as superseding it with a broader errno map and stack capture; worth deciding which one to land.
What was reviewed:
SystemError__toFetchTypeErrorInstance: thescope.clearException()pattern matches the siblingSystemError__toErrorInstanceimmediately above it, so it's consistent with local convention.ValueError::FetchErrorlifecycle:reset/to_js/dupeall handle the new variant symmetrically withSystemError.- Re-checked the three test files flagged in the earlier 🔴 comment (bun-server.test.ts, undici.test.ts, test-http-should-not-accept-untrusted-certificates.ts) plus the html-rewriter regex and the un-awaited
.rejectsin client-fetch — all now updated to match on.code/.causeand awaited.
Extended reasoning...
Overview
Reshapes every fetch() network-error rejection from a bare Error to TypeError('fetch failed', {cause: <SystemError>}), mirroring the Fetch spec (§4.1.12.3) and Node/undici. Native side: a new ValueError::FetchError variant in Body.rs, a SystemError::to_fetch_type_error_instance bridge in SystemError.rs, the C++ SystemError__toFetchTypeErrorInstance in bindings.cpp, and FetchTasklet::on_reject now returns FetchError and maps ConnectionRefused → ECONNREFUSED with syscall: "connect". Fourteen test files updated for the new shape, one new dedicated test file, and one elysia vendor test skipped.
Security risks
None identified. The change is confined to how an already-constructed error is wrapped before rejection; no new user input reaches parsing, allocation, or auth paths.
Level of scrutiny
High. This is a deliberate, ecosystem-visible behavior change to a core Web API on the hot path: .message goes from descriptive to the fixed string "fetch failed", .code for connection-refused changes from "ConnectionRefused" to "ECONNREFUSED", and instanceof TypeError flips. That is the correct direction per spec/Node, but it will break user code that pattern-matches the old shape (the elysia vendor test being the in-tree example). REVIEW.md's API-design guidance calls for maintainer agreement on user-facing surface changes, which this is.
Other factors
- All five of my earlier inline comments (test precision in deinitialization/test.ts,
Buffer.allocvs.repeat, three missed message-asserting tests, un-awaited.rejectsin client-fetch, dead regex arms in html-rewriter) are resolved in the current diff, as is CodeRabbit's un-awaited-rejection batch. - The new C++ function's
clearException()aftertoJSfollows the exact pattern ofSystemError__toErrorInstancea few lines above; not a new violation. - The
ValueError::FetchErrorvariant is wired into every lifecycle method (reset,to_js,dupe) symmetrically withSystemError, so no leak/double-free asymmetry. - The author has since opened #35998, described as covering this reshape plus caller-stack capture, a
'terminated'body-stage message, and a broader errno map. There are also two older overlapping PRs (#35855, #34402). A human should decide whether to land this incrementally or consolidate into #35998.
|
Status (updated): rebased onto current CI on 54f1f27 (#104592): 180/181 jobs passed; every fetch-related test file passed on every lane. The one red job is Verified locally on the rebased build: Ready for review. |
There was a problem hiding this comment.
All prior review feedback has been addressed and CI is green, but this changes the user-visible fetch() rejection shape (Error→TypeError, descriptive message→"fetch failed", ConnectionRefused→ECONNREFUSED) and overlaps with #35998 — a maintainer should sign off on the API surface and decide which PR to land.
What was reviewed:
SystemError__toFetchTypeErrorInstancefollows the sameDECLARE_TOP_EXCEPTION_SCOPE/clearException()pattern as its siblingSystemError__toErrorInstancein the same file.- New
ValueError::FetchErrorvariant is wired into every match site (reset,to_js,dupe). - All 5 previously-flagged test-assertion issues (missing awaits, weakened assertions, three CI-breaking message checks) are fixed in the current diff.
- The
vendor.jsonelysia skip is documented with the upstream fix condition.
Extended reasoning...
Overview
This PR reshapes every fetch() network-error rejection from a bare Error with a descriptive .message into TypeError('fetch failed', {cause: <original Error>}), matching Fetch spec §4.1 step 12.3 and Node/undici. The native change is small (~60 lines across SystemError.rs, bindings.cpp, headers.h, Body.rs, FetchTasklet.rs); the bulk of the diff is 13 test files updated for the new shape plus one new dedicated test file and a vendor.json skip for an elysia test that asserts the old message.
Security risks
None identified. This only changes how already-failing network requests are reported to JS; no new input parsing, no auth/TLS logic changes. The .cause chain preserves all diagnostic fields (.code/.syscall/.path/.hostname) that were previously on the top-level error.
Level of scrutiny
High — user-facing API design. While the implementation is straightforward and well-tested, this is a deliberate behavioral break for a core Web API: existing Bun code checking err.code === 'ConnectionRefused' or err.message.includes('Unable to connect') will stop matching. The PR mitigates this by mirroring .code onto the outer TypeError, but the ConnectionRefused→ECONNREFUSED remap is still a break. REVIEW.md's guidance on API surface changes and the note that #35998 covers a superset of this reshape (plus caller-stack capture, a 'terminated' body-stage message, and a broader errno map) both point to a maintainer decision.
Other factors
- Prior feedback fully addressed: my four earlier inline comments (weakened assertions, harness convention, three would-fail tests, un-awaited
.rejects, html-rewriter regex) and CodeRabbit's await-rejection findings are all resolved in the current diff. - CI green: build #82837 passed on all fetch-related lanes; the two red jobs are documented as unrelated (stale binary-size baseline, worker-thread stress SIGABRT on one ASAN lane).
- C++ exception handling: the new binding's
scope.clearException()usage mirrors the existingSystemError__toErrorInstanceimmediately above it in the same file, so it's consistent with local convention rather than a new pattern. - Enum exhaustiveness: the new
ValueError::FetchErrorarm is present inreset(),to_js(), anddupe()— no missed match sites found. - Competing PR: robobun opened #35998 which extends this same reshape; landing order/scope is a maintainer call.
| // Fresh Map with fresh entry arrays: node's binding materialises a new map | ||
| // per call, and callers may mutate the [name, message] pairs. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // ErrnoException validates err via getSystemErrorName (type + range) and | ||
| // builds node's exact `${syscall} ${code}[ ${original}]` shape. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // JSC manages one undivided heap, so a record carries a single space instead | ||
| // of V8's thirteen, and counters JSC does not track are reported as 0 rather | ||
| // than invented. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // A profiler that is started and then dropped without stop() would otherwise | ||
| // leave its native session open for the life of the VM; the registry releases | ||
| // it when the wrapper is collected, matching node's BaseObject finalizer. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // A JavaScriptCore eden collection only scans newly allocated objects, | ||
| // and a full collection sweeps the whole heap, so they line up with | ||
| // V8's minor and major collection types. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Node derives this tag from the V8 version, command-line flags, and CPU | ||
| // features; Bun mirrors that with its own version plus the flags recorded by | ||
| // setFlagsFromString, so the tag is stable until the flags change. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // V8 divides its heap into fixed spaces; JSC manages one undivided heap, so | ||
| // the JSC totals are reported under "old_space" and the other V8 space names | ||
| // exist for shape compatibility. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // JSC does not expose a per-category code size breakdown; report zeros rather | ||
| // than invented numbers, like node does for counters V8 is not tracking | ||
| // (e.g. cpu_profiler_metadata_size). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // V8 flags have no JSC equivalent; record them so cachedDataVersionTag | ||
| // changes like node's does, and otherwise ignore them. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Bun has no cppgc (Oilpan) C++ heap, so the statistics are always empty; | ||
| // this matches node's shape with nothing allocated through cppgc. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Buffer-bearing payloads are framed as MAGIC + version + SSV([value, buffers]) so deserialize | ||
| // can restore Buffer prototypes (see internal/serialization_buffers). Leading 0xFF cannot collide | ||
| // with bare SSV output; Buffer-free payloads stay bare SSV so older readers keep working. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
4755cf1 to
9da055a
Compare
| /// The rejection shape of a failed `fetch()` in node/undici: | ||
| /// `TypeError("fetch failed", { cause })`, or `"terminated"` once the | ||
| /// response headers had arrived, where `cause` is this error as a plain | ||
| /// `Error`. `.code` is mirrored onto the `TypeError` for code that reads it | ||
| /// there. Consumes `self` like `to_error_instance`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // undici rejects a failed fetch() with TypeError("fetch failed") whose `cause` | ||
| // is the underlying error, and with TypeError("terminated") once the response | ||
| // headers had arrived and the body failed. `cause` is non-enumerable as with | ||
| // `new TypeError(msg, { cause })`. `.code` is mirrored onto the TypeError for | ||
| // code that already reads it there. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// A failed `fetch()`: `TypeError("fetch failed", { cause })`, or | ||
| /// `"terminated"` when the response headers had already arrived (see | ||
| /// `SystemError::to_fetch_failed_instance`). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Once the response headers have been handed out, the failure is one of | ||
| // the body, which undici reports as "terminated" instead of "fetch failed". |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Failures with a libuv equivalent use node's vocabulary (`code`, negative | ||
| // `errno`, `syscall` where the failing call is known); the rest keep | ||
| // their `http::Error` name as the code. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
9da055a to
49529cc
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
49529cc to
2aecba8
Compare
There was a problem hiding this comment.
I reviewed this PR again and found no bugs; every item from my earlier passes (the three missed test files, the un-awaited .rejects at client-fetch.test.ts:518, the weakened assertions in deinitialization/test.ts and html-rewriter.test.js, and the Buffer.alloc convention) has been addressed in the current diff. Because this changes the user-visible fetch() error surface and encodes several API-shape decisions (mirroring .code onto the outer TypeError, the "terminated" vs "fetch failed" split, which http::Error variants get libuv names), a maintainer sign-off on those choices would still be worthwhile.
What was reviewed:
SystemError__toFetchFailedInstance: theclearException()afterBun::toJS(code)matches the identical pattern already used for every field insystemErrorToErrorInstancein the same file.ValueError::FetchFailedin Body.rs:reset/to_js/dupeall handle the new variant;SystemError::clone()oncausebumps refcounts as with the siblingSystemErrorarm.on_rejectin FetchTasklet.rs: both return sites (DNS and general) produce the new variant;terminated = self.metadata.is_some()and theUV_Eerrno mapping look correct.- html-rewriter.test.js:
connectionErroris now an asymmetric matcher passed to.rejects.toThrow(...)at five sites — verified those sites and that the PR ran this file locally.
Extended reasoning...
Overview
The PR reshapes fetch network-error rejections to match Node/undici: TypeError('fetch failed' | 'terminated') with the underlying system error attached as a non-enumerable cause, and .code mirrored onto the outer TypeError for backward compatibility. Native changes span src/jsc/SystemError.rs (extern signature), src/jsc/bindings/bindings.cpp + headers.h (new SystemError__toFetchFailedInstance, removal of the errorType parameter from #35855), src/runtime/webcore/Body.rs (ValueError::SystemTypeError → FetchFailed { cause, terminated }), and src/runtime/webcore/fetch/FetchTasklet.rs (on_reject now emits the new variant with libuv-style errno/syscall for ConnectionRefused/ConnectionClosed). One new test file plus 13 updated test files and a docs paragraph.
Security risks
None identified. The change is purely error-object construction on an already-failing path; no new inputs are parsed, no validation is loosened, and the TLS/redirect/DNS handling itself is unchanged.
Level of scrutiny
High. fetch() is critical user-facing surface, and this PR makes deliberate API-design tradeoffs — which http::Error variants map to libuv codes vs keep Bun's names, mirroring .code on the outer error, choosing not to adopt #35998's per-call stack capture or its broader code renames. The description explains each choice and the consolidation with #34402/#35998, but these are the kind of Node-compat decisions a maintainer should ratify rather than an automated review.
Other factors
All five of my earlier inline threads are resolved and the fixes are visible in the current 20-file diff. The new clearException() in bindings.cpp initially looked like a REVIEW.md violation, but systemErrorToErrorInstance (which the new function calls) already uses the identical DECLARE_TOP_EXCEPTION_SCOPE + per-field clearException() pattern for code/path/syscall/hostname/dest, so this follows the file's local convention. Test coverage is thorough (6 new dedicated cases covering shape, non-enumerable cause, negative errno, both body-read paths, and a hermetic DNS case). The hundreds of comment-cop threads on unrelated files are noise from a stale GitHub diff after a merge and do not concern the 20 files this PR actually touches.
fetch() network failures (connection refused, DNS failure, TLS errors,
socket closed before the response, mid-body truncation, malformed
responses, too many redirects, ...) rejected with a TypeError (since
at the top level and no .cause. Every fetch-error classifier in the
ecosystem keys on the undici shape instead:
- is-network-error (the predicate under p-retry, ky and most hand-rolled
retry loops) requires name === "TypeError" and message === "fetch
failed" / "terminated", so it returned false for every Bun failure and
retry gates never retried.
- The Node idiom err.cause?.code === "ECONNREFUSED" read undefined.
Network errors now reject as TypeError("fetch failed", { cause }), or
TypeError("terminated", { cause }) once the response headers had been
delivered and the body failed, matching undici. The cause is the same
system error Bun used to surface directly (code, path, syscall, errno,
and the descriptive message), so nothing diagnostic is lost, and .code is
mirrored onto the TypeError so existing Bun code reading err.code keeps
working. ConnectionRefused is reported as ECONNREFUSED (syscall
"connect") and ConnectionClosed as ECONNRESET, both with libuv-style
negative errno values; everything else keeps its http::Error name as the
code. Replaces the SystemTypeError variant from #35855.
Fixes #34397
Addresses cases 1 and 2 of #20486
2aecba8 to
54f1f27
Compare
There was a problem hiding this comment.
I reviewed this and found no bugs; the five points I raised on earlier revisions (distinct ECONNRESET/ECONNREFUSED assertions in the deinit fixture, Buffer.alloc over .repeat, the three un-updated err.message tests, the un-awaited .rejects in client-fetch, and the html-rewriter regex) have all been addressed. Since this changes the public fetch() rejection contract and picks one of three competing PR designs, a maintainer sign-off on the chosen shape is still worth having.
What was reviewed:
SystemError__toFetchFailedInstancefollows the sameDECLARE_TOP_EXCEPTION_SCOPE+ best-effortclearExceptionpattern as the siblingsystemErrorToErrorInstanceit wraps.ValueError::FetchFailedis handled into_js,dupe, andreset(via the fallthrough assignment); no variant left unhandled.- Both
on_rejectreturn sites (DNS and general) produce the new shape;terminated = self.metadata.is_some()matches the documented headers-already-delivered semantics. - Every updated
.rejectsassertion is nowawaited.
Extended reasoning...
Overview
This PR reshapes fetch() network-error rejections to match Node/undici: a TypeError('fetch failed' | 'terminated') with the underlying system error attached as a non-enumerable cause, and .code mirrored onto the outer TypeError for backward compatibility. It replaces the ValueError::SystemTypeError variant with ValueError::FetchFailed { cause, terminated }, adds SystemError__toFetchFailedInstance in bindings.cpp (removing the errorType parameter #35855 introduced), maps ConnectionRefused/ConnectionClosed to libuv-style ECONNREFUSED/ECONNRESET with negative errno and syscall, updates 13 test files to the new shape, adds a 6-case dedicated test file, and documents the shape.
Security risks
None identified. The change is confined to how an already-failed request's rejection reason is constructed; no new input parsing, no auth/TLS-decision changes.
Level of scrutiny
High. This is a user-facing API contract change to fetch() — the most-used networking primitive — and it explicitly consolidates and supersedes two competing PRs (#34402, #35998) with documented trade-off decisions (not adopting per-call stack capture, not renaming the remaining Bun-specific codes to undici names). Those are exactly the API-design and PR-consolidation choices a maintainer should confirm rather than an automated reviewer.
Other factors
All five of my earlier inline findings have been addressed in the current diff, and every thread is resolved. The bug-hunting system found nothing this run. The new C++ follows the file's existing TOP_EXCEPTION_SCOPE + clearException pattern for best-effort error-property population, so no new exception-handling concern is introduced. Test coverage is thorough (refused, reset-before-headers, malformed response, terminated via text() and via a body reader, hermetic DNS in a subprocess with proxy env cleared), and several previously un-awaited .rejects assertions were fixed along the way. Deferring solely because the public error-shape decision warrants a human sign-off, not because of any correctness concern.
…austed (#39183) ### Problem - The `Comment Cop` check is red on every claude-labeled PR whenever the repo's GraphQL quota is used up. The step dies before it scans anything: ``` GraphqlResponseError: Request failed due to following response errors: - API rate limit already exceeded for site ID installation. ##[error]Unhandled error: GraphqlResponseError: ... ``` with `x-ratelimit-resource: graphql`, `x-ratelimit-limit: 10000`, `x-ratelimit-remaining: 0` (run [31898901053](https://github.com/oven-sh/bun/actions/runs/31898901053) on #39139). All 33 comment-cop failures on Aug 15 are this error (197 runs succeeded, 171 were skipped); they hit unrelated PRs at the same time because the quota is shared by every workflow run in the repo, and they come back whenever PR volume is high. - Cause, `.github/workflows/comment-cop.yml:124` on main: the step reads the PR's existing review threads with `github.graphql()`, outside any `try`, and dedup and posting both sit behind that call. The REST call just before it (`pulls.listFiles`) had succeeded in each failing run, so the step had the diff; it only lacked the dedup data, which it was reading from the exhausted quota. - The same thread data also fed an auto-resolve of stale threads. That part does not work: under the Actions `GITHUB_TOKEN` every `resolveReviewThread` mutation fails with `Resource not accessible by integration`, after which the step still logs `Resolved N stale comment-cop thread(s).` Two successful runs from today: [31900717034](https://github.com/oven-sh/bun/actions/runs/31900717034) (20 attempted, 20 failed, "Resolved 20") and [31900354924](https://github.com/oven-sh/bun/actions/runs/31900354924) (4 attempted, 4 failed, "Resolved 4"). #36959 documents the same thing and is the PR that moves resolution to a token that can do it. So the GraphQL query was paying for one thing REST can provide and one thing that does not happen. ### Fix - Dedup reads the PR's review comments with `pulls.listReviewComments` (REST) and collects the `<!-- comment-cop:KEY -->` markers from them. Every comment the step posts starts with that marker, so the review comments carry the same keys the thread roots did; REST is the quota the step already needs for `listFiles` and `createReviewComment`, and it was available in every failing run. - The GraphQL query and the resolve loop are removed, so the step makes no GraphQL request at all; the failure in the Problem section cannot happen, rather than being caught. Nothing observable is lost: the mutations the loop issued all fail today, and the only other thing it did was log `Resolved N stale comment-cop thread(s).` after they had failed. Resolving stale threads stays with #36959, which will also need to move its thread lookup into its token step, since this PR removes the `GITHUB_TOKEN` lookup it currently reuses (noted there). - What the step posts is unchanged: the script on main and the REST dedup were dry-run (real reads, writes recorded) against 9 PRs carrying existing comment-cop threads (#35988, #36956, #36713, #35635, #33632, #35596, #39139, #30609, #36959) and chose the same comments to post on every one of them; the canned scenarios below show the same thing with GraphQL working and with it exhausted. - Test: `test/internal/source-lints/comment-cop.test.ts` extracts the script from the workflow and runs it the way `actions/github-script` does, against a fake `github` whose GraphQL requests all fail with the rate limit error above. It checks that groups not yet flagged are posted with the right line ranges, that a group already flagged (and a stale marker) are left alone, that a second run recognizes the comments the first run posted and posts nothing, and that no GraphQL request is made. Both tests fail against the workflow on main (the script throws the error above) and pass with this change. `source-lints.yml` now also triggers on changes to `comment-cop.yml`, so the test runs whenever the script is edited; it is excluded from the Buildkite shards like the rest of that directory. - Also ran: `bun bd test test/internal/source-lints/comment-cop.test.ts`, `bun test test/internal/source-lints/` (whole directory green), prettier on the three files. The comment-cop run on this PR itself still executes the script from main (`pull_request_target`), so the `Source lints` job is the one that exercises the change here. - Does not overlap with #37948 (which groups are flagged) or #38127 (where the file list comes from); both touch other parts of the script. #36959 rewrites the block this PR deletes and will need a rebase either way. ### Background - Comment Cop (`.github/workflows/comment-cop.yml`): on each push to a claude-labeled PR it reads the PR diff, finds multi-line comments added under `src/`, and posts one review comment per comment block. Each bot comment starts with `<!-- comment-cop:KEY -->`, KEY being the file path plus a hash of the block's text; a block whose KEY is already on the PR is not posted again. The check is advisory (not required for merge). - GitHub API quotas: REST and GraphQL requests count against separate hourly quotas (`x-ratelimit-resource` is `core` for REST and `graphql` for GraphQL). For the `GITHUB_TOKEN` Actions hands out, each quota is per repository, so every workflow run in oven-sh/bun draws on the same two pools, and GraphQL-heavy automation (the `gh pr` / `gh issue` / `gh search` commands used by other workflows go through GraphQL) empties the GraphQL pool for everything else when PR volume is high. - Review threads vs review comments: a review thread is GraphQL's grouping of a line comment with its replies, and is the only place a thread's id (what `resolveReviewThread` takes) and its resolved flag exist. `GET /repos/{owner}/{repo}/pulls/{n}/comments` returns every review comment on the PR, including each thread's root comment, so the markers are reachable from REST; only resolving needs GraphQL, and under `GITHUB_TOKEN` GitHub refuses that mutation regardless of the `pull-requests: write` permission. <details> <summary>Script on main vs this branch against a fake github (the fake's resolve mutation fails the way GITHUB_TOKEN's does)</summary> ``` main | quota ok, stale threads present | exit 0 | posts ["src/foo.ts:5-6"] | resolve attempts ["T_STALE_OPEN"] | graphql ["query","query","mutation"] | warnings 1 main | graphql quota exhausted, stale threads present | step fails (unhandled rate limit error) | posts [] | resolve attempts [] | graphql ["query"] | warnings 0 main | graphql quota exhausted, nothing stale | step fails (unhandled rate limit error) | posts [] | resolve attempts [] | graphql ["query"] | warnings 0 main | graphql quota exhausted, PR has no review comments yet | step fails (unhandled rate limit error) | posts [] | resolve attempts [] | graphql ["query"] | warnings 0 fixed | quota ok, stale threads present | exit 0 | posts ["src/foo.ts:5-6"] | resolve attempts [] | graphql [] | warnings 0 fixed | graphql quota exhausted, stale threads present | exit 0 | posts ["src/foo.ts:5-6"] | resolve attempts [] | graphql [] | warnings 0 fixed | graphql quota exhausted, nothing stale | exit 0 | posts ["src/foo.ts:5-6"] | resolve attempts [] | graphql [] | warnings 0 fixed | graphql quota exhausted, PR has no review comments yet | exit 0 | posts ["src/foo.ts:2-3","src/foo.ts:5-6"] | resolve attempts [] | graphql [] | warnings 0 ``` </details> <details> <summary>Dry-run against live PRs: comments the script on main would post vs the REST dedup (real reads through a user token, writes recorded)</summary> ``` PR #35988: same posts (0 vs 0) 245 threads on the PR are stale; main attempts to resolve them, this branch does not PR #36956: same posts (0 vs 0) PR #36713: same posts (1832 vs 1832) the stale cached file list that #38127 fixes; identical on both PR #35635: same posts (0 vs 0) PR #33632: same posts (0 vs 0) PR #35596: same posts (0 vs 0) PR #39139: same posts (1 vs 1) the comment the failing run above did not get to post PR #30609: same posts (0 vs 0) PR #36959: same posts (0 vs 0) ``` </details> <details> <summary>Headers from the failing run</summary> ``` errors: [ { type: 'RATE_LIMIT', code: 'graphql_rate_limit', message: 'API rate limit already exceeded for site ID installation.' } ] variables: { owner: 'oven-sh', repo: 'bun', pr: 39139, after: null } 'x-ratelimit-limit': '10000' 'x-ratelimit-remaining': '0' 'x-ratelimit-resource': 'graphql' 'x-ratelimit-used': '10000' ``` </details>
Problem
A
fetch()that fails on the network rejects with aTypeError(since fetch: mark body disturbed when a reader fails; reject network errors as TypeError #35855) that carries Bun's own description and code at the top level and has nocause:Node code that follows the documented shape (
err.cause?.code === "ECONNREFUSED") never matches on Bun (fetch() connection-refused error shape differs from Node: no e.cause.code === 'ECONNREFUSED' #34397), andis-network-error(the classifier underp-retry,kyand most retry loops), which checksname === "TypeError"plus a fixed set of messages including"fetch failed"and"terminated", returns false for every Bun failure, so those retry loops never retry on Bun (Nativefetchincompatibilities with NodeJS error format and codes #20486).Cause:
FetchTasklet::on_reject(src/runtime/webcore/fetch/FetchTasklet.rs) builds oneSystemErrorand returns it asValueError::SystemTypeError, whichSystemError__toTypeErrorInstancematerialises as a singleTypeErrorwith the description as its message.Fix
ValueError::SystemTypeErrorbecomesValueError::FetchFailed { cause, terminated }(src/runtime/webcore/Body.rs); bothon_rejectreturn sites (the DNS path and the general path) produce it, so every network failure gets the same shape.terminatedisself.metadata.is_some(): the response headers had already been handed out, so the failure is one of the body, which undici reports asTypeError("terminated")instead of"fetch failed".SystemError__toFetchFailedInstance(src/jsc/bindings/bindings.cpp) builds the cause with the existingErrorconstructor (socode,message,path,syscall,hostname,errnoare exactly what Bun surfaced at the top level before, and Bun's error printer shows both levels), createsTypeError("fetch failed" | "terminated"), attaches the cause as a non-enumerable property the waynew TypeError(msg, { cause })does, and mirrorscodeonto theTypeError, so existing Bun code readingerr.codekeeps working anderr.cause?.code ?? err.codeis portable.SystemError__toTypeErrorInstanceand theerrorTypeparameter fetch: mark body disturbed when a reader fails; reject network errors as TypeError #35855 added for it are removed again; nothing else used them.code: "ECONNREFUSED",syscall: "connect",errno: -ECONNREFUSED; a dropped connection staysECONNRESET(already the case onmain) and gains the matching negativeerrno, with nosyscallsince the failing call is not known there. Every other failure keeps itshttp::Errorname as the code (DEPTH_ZERO_SELF_SIGNED_CERT,TooManyRedirects,Malformed_HTTP_Response, ...); DNS failures keep thegetaddrinfoerror Report DNS lookup failures from fetch() and Bun.connect as ENOTFOUND #32990 introduced, now on the cause.TypeError, and node's shape for thatTypeError(message, non-enumerablecause,cause.code) is the one portable code is written against; keeping Bun's description and codes on the cause, plus thecodemirror, means no information that was available before is lost, only moved.test/js/web/fetch/fetch-network-error.test.ts(6 cases, all failing on the released binary): the full shape, includingerrno < 0and the non-enumerablecause, for a refused connection and for a connection dropped before the headers; a malformed response keeping Bun's code on both levels;"terminated"for a body cut short, both throughtext()and through a body-stream reader; and a hermetic DNS failure (a 64-character label, which the resolver rejects locally) in a child process with the proxy variables cleared. The existing tests that asserted the previous message orConnectionRefusedwere updated to assertcode/cause(13 files; a few of them were not awaiting the assertion they made).body-mixin-errors,client-fetch,fetch-redirect,fetch-gzip,fetch.tls,fetch.tls.wildcard,fetch-tls-cert,18413-*,error-name-preservation,undici,html-rewriterand the twotest/js/bun/test/parallel/test-http-*scripts were run locally.docs/runtime/networking/fetch.mdxdescribes the shape.Consolidation
#34402 and #35998 were alternative fixes for the same issues. Folded in from #34402:
syscall/negativeerrnoon the cause and the check thatcauseis non-enumerable. Folded in from #35998: the"terminated"message for body-stage failures and the hermetic DNS test. Not adopted from #35998: capturing anErrorat everyfetch()call to give the rejection a caller stack (node does not do this either, itsfetch failedstack has no caller frame unless anawaitchain supplies one, which Bun's existing async-stack attachment also provides; the capture costs an allocation per successfulfetch()and had to weaken theserve.test.tsguard against per-requestErrorallocations), and its renaming of Bun's remaining codes to undici/http-parser names (Timeout -> ETIMEDOUTis unreachable because timeouts becomeTimeoutErrorfirst,InvalidContentLength -> UND_ERR_RES_CONTENT_LENGTH_MISMATCHdescribes a different failure, and the rest are approximations that would break the existingcodechecks for no portability gain). Thestackbeing absent on errors created with no JS frames affects all natively created errors, not onlyfetch(), and is tracked separately.Not covered here, as in the other two PRs: cases 3 and 4 of #20486 (an invalid URL getting a
causewithERR_INVALID_URL, an unsupported scheme) still reject as they did before, so that issue is left open; cases 1 and 2 are what this PR addresses.Background
ValueError(Body.rs): the lazily materialised rejection reason stored on a body; it is turned into a JS value (to_js) only when something reads the body or the fetch promise is rejected, and can be duplicated when aResponseis cloned, which is why it is a Rust enum rather than a JS value.SystemError(src/jsc/SystemError.rs): the#[repr(C)]struct (code,message,path,syscall,hostname,errno, ...) that C++ turns into a JS error object; Rust keeps ownership of the strings, C++ only reads them.metadataon the tasklet is set once the HTTP client has delivered the final response's status and headers; redirects followed internally never set it.Fixes #34397
Addresses cases 1 and 2 of #20486
no test proof · iteration 7 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/workerd/html-rewriter.test.js, test/js/web/fetch/fetch.test.ts, test/js/bun/http/serve.test.ts, test/js/bun/http/bun-server.test.ts