Skip to content

node: fix broken error messages for ERR_HTTP2_UNSUPPORTED_PROTOCOL and 4 others - #35791

Closed
robobun wants to merge 5 commits into
mainfrom
farm/ec9fe768/node-error-message-templates
Closed

robobun wants to merge 5 commits into
mainfrom
farm/ec9fe768/node-error-message-templates

Conversation

@robobun

@robobun robobun commented Jul 25, 2026 •

Copy link
Copy Markdown
Collaborator

What

jsFunctionMakeErrorWithCode (ErrorCode.cpp) has message templates for ~134 of the 346 registered ERR_* codes. Every other code falls through to the tail which uses callFrame->argument(1).toWTFString() verbatim as .message. A JS call site that passes template args instead of a full sentence yields a garbage message, and a call site that passes a full sentence into a code that is templated gets double-wrapped.

Repro

import http2 from "node:http2";
try { http2.connect("gopher://127.0.0.1:1"); } catch (e) { console.log(e.code, JSON.stringify(e.message)); }
// bun:  ERR_HTTP2_UNSUPPORTED_PROTOCOL "gopher:"
// node: ERR_HTTP2_UNSUPPORTED_PROTOCOL "protocol \"gopher:\" is unsupported."

const http = require("node:http");
http.createServer((q, res) => { res.destroy(); res.write("x", err => { console.log(err.code, JSON.stringify(err.message)); process.exit(0); }); })
  .listen(0, function () { require("node:net").connect(this.address().port).write("GET / HTTP/1.1\r\nHost: x\r\n\r\n"); });
// bun:  ERR_STREAM_DESTROYED "Cannot call Stream is destroyed after a stream was destroyed"
// node: ERR_STREAM_DESTROYED "Cannot call write after a stream was destroyed"

Fix

code before after
ERR_HTTP2_UNSUPPORTED_PROTOCOL "gopher:" protocol "gopher:" is unsupported.
ERR_HTTP_TRAILER_INVALID "undefined" Trailers are invalid with this transfer encoding
ERR_HTTP_CONTENT_LENGTH_MISMATCH (client OutgoingMessage) "11" Response body's content-length of 11 byte(s) does not match the content-length of 5 byte(s) set in header
ERR_HTTP_CONTENT_LENGTH_MISMATCH (server ServerResponse, native) "Content-Length mismatch" same as above
ERR_STREAM_DESTROYED (http ServerResponse) Cannot call Stream is destroyed after a stream was destroyed Cannot call write after a stream was destroyed
ERR_OPERATION_FAILED (fs.promises writeFile) Operation failed: Operation failed: write failed after retries Operation failed: write failed after retries
ERR_SCRIPT_EXECUTION_INTERRUPTED (REPL) "undefined" Script execution was interrupted by `SIGINT`
  • Added C++ message templates for ERR_HTTP2_UNSUPPORTED_PROTOCOL, ERR_HTTP_TRAILER_INVALID, ERR_HTTP_CONTENT_LENGTH_MISMATCH, ERR_SCRIPT_EXECUTION_INTERRUPTED.
  • NodeHTTPResponse.rs: format the actual/expected byte counts into the same Node template (the server-side check throws via the native err_throw path and doesn't reach jsFunctionMakeErrorWithCode).
  • internal/http.ts: $ERR_STREAM_DESTROYED("Stream is destroyed") → $ERR_STREAM_DESTROYED("write") (the code already has a "Cannot call <arg> after a stream was destroyed" template).
  • fs.promises.ts (3 sites): drop the redundant "Operation failed: " prefix (the template already prepends it).
  • _http_server.ts (2 sites): drop now-redundant message arg from $ERR_HTTP_TRAILER_INVALID(...).
  • Debug-only guard on the fallthrough tail: throw if the first argument is not a string, so future mismatches surface at test time. Swept all $ERR_*(...) call sites in src/js/ (including the ...args spread wrappers in internal/repl/node-errors.js); none pass a non-string through the fallthrough with these templates in place.

Supersedes #35777 (two of the six codes) and #17927 (old src/bun.js/ path).

Verification

bun bd test test/js/node/node-error-messages.test.ts
 5 pass, 0 fail

With src/ reverted to origin/main:

 0 pass, 5 fail

[review] gate passed · iteration 2 · 6 files touched

fails on main (without fix)
ASAN without fix: 5 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/node-error-messages.test.ts
bun test v1.4.0 (b832defec)

test/js/node/node-error-messages.test.ts:
17 |     http2.connect("gopher://127.0.0.1:1");
18 |   } catch (e) {
19 |     err = e;
20 |   }
21 |   expect(err?.code).toBe("ERR_HTTP2_UNSUPPORTED_PROTOCOL");
22 |   expect(err?.message).toBe('protocol "gopher:" is unsupported.');
                            ^
error: expect(received).toBe(expected)

Expected: "protocol "gopher:" is unsupported."
Received: "gopher:"

      at <anonymous> (/workspace/bun/test/js/node/node-error-messages.test.ts:22:24)
(fail) ERR_HTTP2_UNSUPPORTED_PROTOCOL message matches Node.js [99.43ms]
32 |     req.flushHeaders();
33 |   } catch (e) {
34 |     err = e;
35 |   }
36 |   req.destroy();
37 |   expect({ code: err?.code, message: err?.message }).toEqual({
                                                          ^
error: expect(received).toEqual(expected)

  {
    "code": "ERR_HTTP_TRAILER_INVALID",
-   "message": "Trailers are invalid with this transfer encoding",
+   "message": "undefined",
  
... (truncated)

release without fix: 1 FAILED
bun test v1.4.0-canary.1 (5c12a3501)

test/js/node/node-error-messages.test.ts:
(pass) ERR_HTTP2_UNSUPPORTED_PROTOCOL message matches Node.js [2.89ms]
(pass) ERR_HTTP_TRAILER_INVALID message matches Node.js [7.13ms]
(pass) ERR_HTTP_CONTENT_LENGTH_MISMATCH message matches Node.js [1.00ms]
81 |   sock.on("error", () => {});
82 |   sock.write("GET / HTTP/1.1\r\nHost: x\r\n\r\n");
83 |   const err = await promise;
84 |   sock.destroy();
85 |   await new Promise<void>(r => server.close(() => r()));
86 |   expect({ code: err?.code, message: err?.message }).toEqual({
                                                          ^
error: expect(received).toEqual(expected)

  {
    "code": "ERR_HTTP_CONTENT_LENGTH_MISMATCH",
-   "message": "Response body's content-length of 11 byte(s) does not match the content-length of 5 byte(s) set in header",
+   "message": "Content-Length mismatch",
  }

- Expected  - 1
+ Received  + 1

      at <anonymous> (/workspace/bun/test/js/node/node-error-messages.test.ts:86:54)
(fail) ERR_HTTP_CONTENT_LENGTH_MISMATCH message from http ServerResponse matches Node.js [21.44ms]
(pass) ERR_STREAM_DESTROYED message from http ServerResponse matches Node.
... (truncated)
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/node-error-messages.test.ts
bun test v1.4.0 (b832defec)

test/js/node/node-error-messages.test.ts:
(pass) ERR_HTTP2_UNSUPPORTED_PROTOCOL message matches Node.js [66.54ms]
(pass) ERR_HTTP_TRAILER_INVALID message matches Node.js [284.55ms]
(pass) ERR_HTTP_CONTENT_LENGTH_MISMATCH message matches Node.js [34.54ms]
(pass) ERR_HTTP_CONTENT_LENGTH_MISMATCH message from http ServerResponse matches Node.js [919.17ms]
(pass) ERR_STREAM_DESTROYED message from http ServerResponse matches Node.js [127.23ms]

 5 pass
 0 fail
 6 expect() calls
Ran 5 tests across 1 file. [6.66s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 1621ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/80] gen ErrorCode+*.h
[2/33] gen cpp.rs (cppbind)
[3/33] gen ZigGeneratedClasses.{cpp,h,rs}
Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts
  - ResolveMessage (13 fields)
  - BuildMessage (10 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts
  - Archive (4 fields, 1 class fields)
Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts
  - ResourceUsage (8 fields)
  - Subprocess (20 fields)
Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts
  - CronJob (5 fields)
Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts
  - FileSystemRouter (5 fields)
  - FrameworkFileSystemRouter (2 fields)
  - MatchedRoute (8 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Glob.classes.ts
  - Glob (5 fields)
Found 1 classes from /workspace/bun/src/runtime/api/h2.classes.ts
  - H2FrameParser (31 fields)
Found 8 classes from /workspace/bun/src/runtime/api/html_rewriter.classes.ts
  - HTMLRewriter (3 f
... (truncated)
diff hotspot
src/js/internal/http.ts                  |   2 +-
 src/js/node/_http_server.ts              |   4 +-
 src/js/node/fs.promises.ts               |   6 +-
 src/jsc/bindings/ErrorCode.cpp           |  32 ++++++++-
 src/runtime/server/NodeHTTPResponse.rs   |  30 +++++++--
 test/js/node/node-error-messages.test.ts | 111 +++++++++++++++++++++++++++++++
 6 files changed, 172 insertions(+), 13 deletions(-)

gate history · 3 passed · 1 rejected · iteration 2

evidence per changed file
file                                      reads  edits  tests
src/js/internal/http.ts                       1      1      0
src/js/node/_http_server.ts                   1      1      0
src/js/node/fs.promises.ts                    1      1      0
src/jsc/bindings/ErrorCode.cpp                4      5      0
src/runtime/server/NodeHTTPResponse.rs        4      4      0
test/js/node/node-error-messages.test.ts      1      5      0

…and friends

jsFunctionMakeErrorWithCode falls through to 'use arg[0] verbatim as .message'
for any code without an explicit template case. Call sites that pass template
args (not a full sentence) produce garbage messages; call sites that pass a
full sentence into a code that is templated get the wrapper applied twice.

Add templates for ERR_HTTP2_UNSUPPORTED_PROTOCOL, ERR_HTTP_TRAILER_INVALID
and ERR_HTTP_CONTENT_LENGTH_MISMATCH, fix the ERR_STREAM_DESTROYED call in
internal/http.ts to pass the method name, and drop the duplicated
'Operation failed: ' prefix from the three ERR_OPERATION_FAILED call sites
in fs.promises.ts.

Also add a debug-only check to the fallthrough tail that throws when the
first argument is not a string, so future template/arg mismatches surface
at test time instead of shipping a broken message.
@coderabbitai

coderabbitai Bot commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 5 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 645994c9-bd0a-4b42-b6eb-117abd092b2e

📥 Commits

Reviewing files that changed from the base of the PR and between 04bb5c4 and b832def.

📒 Files selected for processing (6)
  • src/js/internal/http.ts
  • src/js/node/_http_server.ts
  • src/js/node/fs.promises.ts
  • src/jsc/bindings/ErrorCode.cpp
  • src/runtime/server/NodeHTTPResponse.rs
  • test/js/node/node-error-messages.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jul 25, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 9:26 PM PT - Jul 25th, 2026

❌ @robobun, your commit b832def has 1 failures in Build #81758 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 35791

That installs a local version of the PR into your bun-35791 executable, so you can run:

bun-35791 --bun

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. node:http: fix ERR_HTTP_CONTENT_LENGTH_MISMATCH and ERR_HTTP_TRAILER_INVALID message text #35777 - also fixes ERR_HTTP_CONTENT_LENGTH_MISMATCH and ERR_HTTP_TRAILER_INVALID message text (2 of the same 5 error codes)

🤖 Generated with Claude Code

Comment thread src/jsc/bindings/ErrorCode.cpp
Comment thread src/jsc/bindings/ErrorCode.cpp
Comment thread test/js/node/node-error-messages.test.ts

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — a02533b addresses both prior findings: the ERR_SCRIPT_EXECUTION_INTERRUPTED fixed-message case is added (so the debug guard no longer trips on the REPL zero-arg path), and the two dead _http_server.ts args are dropped.

What was reviewed:

  • Verified all JS call sites for the 5 templated codes now match their C++ arg signatures (ERR_HTTP_CONTENT_LENGTH_MISMATCH two-arg at _http_outgoing.ts:934/1079, ERR_HTTP2_UNSUPPORTED_PROTOCOL one-arg at http2.ts:5528, ERR_HTTP_TRAILER_INVALID argless at all 3 sites).
  • Re-checked node-errors.js spread wrappers against the switch: with ERR_SCRIPT_EXECUTION_INTERRUPTED now handled, no remaining zero-arg fallthrough reaches the new ASSERT_ENABLED guard.
  • New C++ cases follow the neighboring pattern (toWTFString + RETURN_IF_EXCEPTION before use).
Extended reasoning...

Overview

Fixes five broken .message strings for Node.js ERR_* codes by adding message templates to jsFunctionMakeErrorWithCode (ErrorCode.cpp) and correcting the JS-side call arguments in internal/http.ts, _http_server.ts, and fs.promises.ts. Adds a debug-only guard on the fallthrough tail so future template/call-site mismatches surface at test time. Ships four subprocess tests pinning the messages to Node.js's format.

Changes since my previous review

Commit a02533b addresses both actionable findings from my earlier pass: (1) the ERR_SCRIPT_EXECUTION_INTERRUPTED fixed-message case is now present, so the new ASSERT_ENABLED guard no longer fires on the REPL SIGINT-during-await path (repl.js:656 → node-errors.js ...args wrapper → zero-arg fallthrough); (2) the two now-dead string arguments at _http_server.ts:2117/2147 are dropped, matching _http_outgoing.ts:563. The remaining nit — a test-file comment noting why ERR_OPERATION_FAILED in FileHandle.writer is untestable — was explicitly non-blocking and is fine to skip.

Security risks

None. This is error-message string plumbing; no auth, crypto, parsing of untrusted input, or resource-limit changes. The debug guard is #if ASSERT_ENABLED only and does not run in release builds.

Level of scrutiny

Low-to-medium. The C++ additions are mechanical copies of the neighboring switch-case pattern (same toWTFString → RETURN_IF_EXCEPTION → makeString → createError sequence), and the JS edits are string-literal changes. I re-verified every $ERR_* call site for the six affected codes against the new/existing templates: ERR_HTTP_CONTENT_LENGTH_MISMATCH (2 args, both sites), ERR_HTTP2_UNSUPPORTED_PROTOCOL (1 arg), ERR_HTTP_TRAILER_INVALID (0 args, all 3 sites), ERR_SCRIPT_EXECUTION_INTERRUPTED (0 args via spread), ERR_STREAM_DESTROYED (1 arg = method name), ERR_OPERATION_FAILED (1 arg, prefix dropped). All line up.

Other factors

The bug-hunting system found nothing new this run. Tests follow harness conventions (bunExe/bunEnv, test.concurrent for the subprocess cases, combined {stdout, stderr, exitCode} assertion). A github-actions bot flagged possible overlap with #35777 on two of the codes; that's a merge-order decision, not a correctness concern for this diff.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the follow-up commit addressed the ERR_SCRIPT_EXECUTION_INTERRUPTED guard trip and dropped the dead _http_server.ts args; all three prior findings are resolved.

What was reviewed:

  • New C++ template cases match their JS call sites (_http_outgoing.ts:934/1079 passes two numeric args, http2.ts:5528 passes the protocol string, all three ERR_HTTP_TRAILER_INVALID sites now argless).
  • ERR_OPERATION_FAILED and ERR_STREAM_DESTROYED templates confirmed to already exist in ErrorCode.cpp, so the JS-side arg edits produce the intended messages.
  • Debug-only fallthrough guard: verified the remaining ...args spread wrappers in node-errors.js all have template cases and won't reach it with undefined.
Extended reasoning...

Overview

Fixes six Node.js ERR_* error messages that were malformed because jsFunctionMakeErrorWithCode either lacked a template (so the raw arg became .message) or already had one (so the arg got double-wrapped). Changes: four new switch cases in ErrorCode.cpp (ERR_HTTP2_UNSUPPORTED_PROTOCOL, ERR_HTTP_CONTENT_LENGTH_MISMATCH, ERR_HTTP_TRAILER_INVALID, ERR_SCRIPT_EXECUTION_INTERRUPTED), a debug-only guard on the fallthrough tail, string-literal fixes at 6 JS call sites across internal/http.ts / _http_server.ts / fs.promises.ts, and a new 4-test file pinning the messages to Node's format.

Security risks

None. This is pure error-message string formatting for Node.js compat. No auth, crypto, parsing of untrusted input, or memory management is touched. The C++ additions follow the exact toWTFString + RETURN_IF_EXCEPTION pattern of the ~130 neighboring cases.

Level of scrutiny

Low-to-medium. The change is mechanical: each new C++ case is copy-paste of the adjacent template pattern with a Node-matched format string, and each JS edit is a string-literal argument change. I cross-checked every affected call site against its template: ERR_HTTP_CONTENT_LENGTH_MISMATCH receives two numeric args (toWTFString handles numbers), ERR_HTTP2_UNSUPPORTED_PROTOCOL receives the protocol string, ERR_STREAM_DESTROYED already has a Cannot call {arg} after... template so "write" is the correct arg, and ERR_OPERATION_FAILED already prepends Operation failed: so dropping the JS-side prefix is correct.

Other factors

The first review round flagged one real regression (the new ASSERT_ENABLED guard would fire on ERR_SCRIPT_EXECUTION_INTERRUPTED from repl.js:656) and two nits (dead args in _http_server.ts, missing test for the unreachable ERR_OPERATION_FAILED path). The author fixed the blocker and one nit in a02533b, and the remaining nit is acknowledged as effectively untestable without fault injection. All three inline threads are resolved. Tests are verified to fail on the unfixed build and pass on the PR build; three of the four tests use test.concurrent with subprocess spawning per harness conventions.

…esses

The subprocess-based tests each paid a ~2s ASAN bun-debug boot, and with
three running concurrently the ERR_STREAM_DESTROYED case exceeded the 5s
default under load. The same code paths are reachable in-process; each
test now runs in <1s under ASAN.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the ERR_SCRIPT_EXECUTION_INTERRUPTED template and the _http_server.ts dead-arg cleanup from a02533b address the earlier findings; the remaining inline note is pre-existing and out of this PR's mechanism scope.

Extended reasoning...

Overview

Adds four message-template cases to jsFunctionMakeErrorWithCode in ErrorCode.cpp (ERR_HTTP2_UNSUPPORTED_PROTOCOL, ERR_HTTP_CONTENT_LENGTH_MISMATCH, ERR_HTTP_TRAILER_INVALID, ERR_SCRIPT_EXECUTION_INTERRUPTED), a debug-only isString() guard on the fallthrough tail, and six string-literal-only edits at JS call sites (internal/http.ts, _http_server.ts, fs.promises.ts) to match existing templates. Four in-process tests pin the messages to Node's format.

Security risks

None. Error-message string content only; no auth, parsing, allocation, or user-input handling changes.

Level of scrutiny

Low-to-medium. The C++ additions are mechanical copies of the neighboring template pattern (each toWTFString followed by RETURN_IF_EXCEPTION). The debug-only guard was the one structural change; my previous review caught its one live landmine (ERR_SCRIPT_EXECUTION_INTERRUPTED via the ...args spread wrapper), which was fixed in a02533b, and I re-verified the other spread wrappers in node-errors.js (ERR_USE_AFTER_CLOSE, ERR_INVALID_CURSOR_POS) have switch cases. I also confirmed the _http_outgoing.ts and http2.ts call sites pass argument shapes matching the new templates.

Other factors

All three threads from the earlier review round are resolved — two by code change, one (untested ERR_OPERATION_FAILED path) was explicitly non-blocking. The evidence block shows the tests fail on unpatched debug/ASAN and pass on both patched debug/ASAN and release. This run's remaining nit — the server-side NodeHTTPResponse.rs err_throw path for ERR_HTTP_CONTENT_LENGTH_MISMATCH — is a separate error-construction mechanism that never enters jsFunctionMakeErrorWithCode, is pre-existing, and already produces a coherent (if not byte-identical) message; it doesn't block this PR.

Comment thread src/jsc/bindings/ErrorCode.cpp
… format

The server-side ServerResponse strictContentLength check throws via the
native err_throw path and never reaches jsFunctionMakeErrorWithCode, so it
still produced 'Content-Length mismatch' instead of Node's templated
string. Format the actual/expected byte counts into the same message the
client-side _http_outgoing.ts path now produces.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — all prior feedback addressed.

What was reviewed:

  • Verified _http_outgoing.ts call sites pass (actual, expected) in the order the new C++ template reads them, and http2.ts passes only the protocol string.
  • Confirmed ERR_SCRIPT_EXECUTION_INTERRUPTED now has a fixed-message case so the debug-only fallthrough guard cannot fire on the REPL zero-arg wrapper; other ...args wrappers in node-errors.js all have templates.
  • Checked the new Rust err_throw_content_length_mismatch helper follows the existing global.err(code, format_args!(...)).throw() shape and both call sites pass (bytes_written, content_length) in Node's order.
  • All three $ERR_HTTP_TRAILER_INVALID sites are now argless.
Extended reasoning...

Overview

Node-compat error-message fixes across 6 files: four new switch cases in jsFunctionMakeErrorWithCode (ErrorCode.cpp) supplying Node's message templates for ERR_HTTP_CONTENT_LENGTH_MISMATCH, ERR_HTTP2_UNSUPPORTED_PROTOCOL, ERR_HTTP_TRAILER_INVALID, and ERR_SCRIPT_EXECUTION_INTERRUPTED; a debug-only isString() guard on the fallthrough tail; a one-word arg fix at the $ERR_STREAM_DESTROYED site in internal/http.ts; three string-literal edits in fs.promises.ts dropping a redundant prefix; two now-dead string args removed from _http_server.ts; a cold Rust helper in NodeHTTPResponse.rs formatting the server-side content-length mismatch message; and a new 5-test file.

Security risks

None. Pure error-message text/formatting changes with no parsing, auth, crypto, or resource-handling implications. The debug-only guard is compiled out in release.

Level of scrutiny

Low-to-moderate. The C++ additions are copy-paste of the neighboring template pattern (toWTFString + RETURN_IF_EXCEPTION + makeString + createError); the Rust helper mirrors the existing err_throw_cold. The JS changes are string-literal edits. The main risk was call-site/template argument mismatch, which I traced for each affected code.

Other factors

This is my third pass on the PR. Round 1 caught the ERR_SCRIPT_EXECUTION_INTERRUPTED zero-arg spread wrapper tripping the new debug guard, plus two cleanup nits — all fixed in a02533b. Round 2 caught the server-side NodeHTTPResponse.rs content-length path still using the old message — fixed in b832def with a matching test. This round the bug hunter found nothing. All 5 tests pin exact .code + .message against Node's format; the PR description confirms 5 pass / 0 fail with the fix and 0 pass / 5 fail with src/ reverted. The one remaining nit (no test for the ERR_OPERATION_FAILED prefix drop) was already discussed and accepted as untestable without fault injection.

@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: the diff builds and tests green on the lanes that ran (alpine x64 full test suite: 20/20 shards passed; linux-x64-musl, freebsd x64/aarch64 builds passed). The linux-x64-build-bun job timed out after the Rust compile finished (Finished release profile in 1m 50s), waiting on linux-x64-build-cpp which never got an agent (state=scheduled, started=None). Same agent-availability issue as build 81388 where every lane expired. Ready for review.

@robobun

robobun commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-25, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.

@robobun robobun closed this Sep 13, 2026
dylan-conway pushed a commit that referenced this pull request Sep 19, 2026
### Problem

- Seven Node error codes have a broken `.message`. `http.request({
headers: { Trailer: "X-T" } }).end()` throws `ERR_HTTP_TRAILER_INVALID`
with the message `undefined`. `url.fileURLToPath(httpUrl, { windows:
true })` throws `ERR_INVALID_URL_SCHEME` with the message `file`. Notes
list all seven.
- The cause is the tail of `jsFunctionMakeErrorWithCode`
(`src/jsc/bindings/ErrorCode.cpp:2478`). A code with no message template
uses its first argument as the whole message. These call sites pass
Node's template arguments, or nothing.
- The reverse also happens. A sentence passed to a templated code gives
`Cannot call Stream is destroyed after a stream was destroyed`.

### Fix

- Four codes get Node's template: `ERR_HTTP_TRAILER_INVALID`,
`ERR_SCRIPT_EXECUTION_INTERRUPTED`, `ERR_HTTP_CONTENT_LENGTH_MISMATCH`,
`ERR_INVALID_URL_SCHEME`. The server `strictContentLength` check throws
from Rust (`NodeHTTPResponse.rs`). It now formats Node's sentence with
both byte counts.
- The call sites of three codes now pass Node's argument:
`ERR_STREAM_DESTROYED("write")`, `ERR_OPERATION_FAILED("write failed
after retries")`, `ERR_METHOD_NOT_IMPLEMENTED("FileHandle with fs")`.
Dead arguments are gone.
- Verified: `test/js/node/errors/error-code-messages.test.ts`. Bun
1.4.3-canary.1+367d939d9 fails 7 of its 8 tests. The expected messages
are node v26.3.0's.
- Self-reviewed: 12 concerns raised, 8 addressed, 4 rejected (see
Notes).

### Background

- Built-in JS writes `$ERR_FOO(a, b)`. The build turns it into a call of
`jsFunctionMakeErrorWithCode`, which builds the message in C++.
- A message template is a `case` in that function, or a row in its
`simpleErrorMessages` table: fixed text around one or two arguments.
- `strictContentLength` makes `node:http` throw when the body size
differs from `Content-Length`. The client checks in JS. The server
checks in native code, which never reaches the C++ template.

<details><summary>Notes</summary>

Before and after, per code. The "after" text is identical to node
v26.3.0.

| code | call | before | after |
| --- | --- | --- | --- |
| `ERR_HTTP_TRAILER_INVALID` | client request with a `Trailer` header
and no chunked body | `undefined` | `Trailers are invalid with this
transfer encoding` |
| `ERR_HTTP_CONTENT_LENGTH_MISMATCH` | client `req.end("abc")`,
`Content-Length: 5` | `3` | `Response body's content-length of 3 byte(s)
does not match the content-length of 5 byte(s) set in header` |
| `ERR_HTTP_CONTENT_LENGTH_MISMATCH` | server `res.end("abc")`,
`Content-Length: 5` | `Content-Length mismatch` | the same sentence |
| `ERR_INVALID_URL_SCHEME` | `fileURLToPath(httpUrl, { windows })` |
`file` | `The URL must be of scheme file` |
| `ERR_STREAM_DESTROYED` | `res.destroy(); res.write("x", cb)` | `Cannot
call Stream is destroyed after a stream was destroyed` | `Cannot call
write after a stream was destroyed` |
| `ERR_SCRIPT_EXECUTION_INTERRUPTED` | REPL, Ctrl+C during `await` |
`undefined` | ``Script execution was interrupted by `SIGINT` `` |
| `ERR_OPERATION_FAILED` | `FileHandle` writer, every write returns 0
bytes | `Operation failed: Operation failed: write failed after retries`
| `Operation failed: write failed after retries` |
| `ERR_METHOD_NOT_IMPLEMENTED` | `createReadStream(null, { fd:
fileHandle, fs })` | `The fs.FileHandle with custom fs operations method
is not implemented` | `The FileHandle with fs method is not implemented`
|

- How the list was made: a sweep over `src/js` for every `$ERR_X(` call,
split by whether `X` has a `case` or a table row in `ErrorCode.cpp`. 57
codes have no template. All of them pass a full sentence, except the
four above and `ERR_HTTP2_UNSUPPORTED_PROTOCOL`. The reverse direction
(a sentence passed into a template) gave `ERR_STREAM_DESTROYED` and
`ERR_OPERATION_FAILED`. `ERR_METHOD_NOT_IMPLEMENTED` passes a fragment,
but not the one Node passes.
- The ported `test-fs-read-stream-file-handle.js` gets its upstream
`message:` assertion back. It was commented out because of the
`ERR_METHOD_NOT_IMPLEMENTED` text.
- Deleted arguments: the sentence in two
`$ERR_HTTP_TRAILER_INVALID(...)` calls in `_http_server.ts`, and the
`...args` of the REPL wrapper for `ERR_SCRIPT_EXECUTION_INTERRUPTED`.
The constant-message `case` ignores them.
- `fileURLToPathBuffer` (`url.ts:1320`) passed the whole sentence and
was correct. With the new table row it passes `"file"`, like its sibling
and like Node. Without that edit the message would read `The URL must be
of scheme The URL must be of scheme file`.
- `src/js/builtins.d.ts` declares the argument shapes of the four codes
that got a template. Without a declaration the code generator emits
`(message: string)`.
- Not in this PR: `ERR_HTTP2_UNSUPPORTED_PROTOCOL`
(`http2.connect("ftp://...")` prints `ftp:`) has the same cause. A
separate change owns it (branch
`robobun/d80742c6/http2-unsupported-protocol`), so this PR does not add
its row. #43087 also edits `makeSimpleErrorMessage` and adds a row at
the end of the table. The new rows here are in the middle of the table
to keep the merges clean.
- Not in this PR: `fetch()` also throws
`ERR_HTTP_CONTENT_LENGTH_MISMATCH` (`FetchTasklet.rs`) with its own
sentence about the request body. That is a Bun `fetch` error, not a
`node:http` one, so its text stays.
- Not in this PR: `fs.readFileSync(new URL("http://example.com"))`
throws `ERR_INVALID_URL_SCHEME` from `src/runtime/node/types.rs` with
the text `URL must be a non-empty "file:" path`.
`ERR_INVALID_FILE_URL_PATH` and `ERR_INVALID_FILE_URL_HOST` share that
same sentence there. A separate change fixes the three together, because
the path and host texts need more than a new literal.
- Three of the five `ERR_OPERATION_FAILED` call sites doubled the
prefix, all in the `FileHandle` writer. The other two already pass
Node's argument. The test reaches the synchronous writer site: it
replaces `fs.writeSync` with a function that returns 0, and the writer
looks `writeSync` up on the public module at call time. The two async
sites bind `write` and `writev` at module load, so a test cannot make
them return 0. They get the same change. Node's own writer calls its
binding directly, so this expected string comes from Node's source
(`'Operation failed: %s'` with `'write failed after retries'`) and not
from a Node run. Every other expected string is Node's output for the
same call.
- The REPL test asserts that the output contains the message, not the
whole line. Bun prints `Uncaught Error: <message>` where Node prints
`Uncaught:` and the inspected error with its `[ERR_...]` bracket. That
difference is about the stack header, not the message.
- Node does not check the first `res.write()` against `Content-Length`
(its `_contentLength` is still null at that point). Bun's native check
does. The tests use a second write, which both runtimes reject with `6
byte(s)`.
- Found on the way and not changed here (#43520). Four templates in
`ErrorCode.cpp` differ from Node's own text: `ERR_HTTP_SOCKET_ASSIGNED`
(`Socket already assigned`, Node: `ServerResponse has an already
assigned socket`), `ERR_TLS_INVALID_PROTOCOL_VERSION` and
`ERR_TLS_PROTOCOL_VERSION_CONFLICT` (Node formats the values with `%j`,
so it prints `"TLSv9" is not a valid minimum TLS protocol version`), and
`ERR_IPC_CHANNEL_CLOSED` (`Channel closed.`, Node has no period). Those
are wrong templates, not call sites that miss a template. A script
compared the 118 constant and table messages with the literal templates
in Node's `lib/internal/errors.js`. 108 are identical, and these four
differ.
- Found on the way and not changed here (#43519). The server
`strictContentLength` check differs from Node in behavior:
`Content-Length: 0` is never checked, the first `res.write()` is checked
(Node checks from the second write on), and a string header is parsed
with `parseInt` where Node uses `+value`.
- Self-review, the four concerns I did not act on. (1) Fix the
`types.rs` arm of `ERR_INVALID_URL_SCHEME` here: a separate change fixes
the three arms together. (2) Add the `ERR_HTTP2_UNSUPPORTED_PROTOCOL`
row here: a separate change owns it. (3) Fix `ERR_HTTP_SOCKET_ASSIGNED`
and the `Content-Length` parsing here: they are a different class,
tracked in #43520 and #43519. (4) The REPL test passes a 20 s timeout,
and `test/CLAUDE.md` says not to set one: `node:repl` takes 5 to 9 s to
load on a debug build with ASAN, and `test/js/bun/repl/repl.test.ts`
uses the same value for the same reason.
- Earlier work: #35791 covered five of these codes and was closed as
stale with conflicts, not on its merits. #35777 covered two. This PR
follows the review threads of #35791: the server path, the REPL code,
and the dead arguments.
- Suites run on the debug build: `error-code-messages.test.ts`,
`test/js/node/url/url-fileurltopath*.test.*`,
`node-http-transfer-encoding.test.ts`, `node-http.test.ts`,
`test/js/node/fs/promises.test.js`, and the ported
`test-http-content-length-mismatch.js`,
`test-http-server-de-chunked-trailer.js`, `test-http-set-trailers.js`,
`test-url-fileurltopath.js`, `test-fs-whatwg-url.js`,
`test-fs-read-stream-file-handle.js`, `test-repl-sigint.js`,
`test-repl-sigint-nested-eval.js`, `test-worker-unsupported-path.js`. In
`node-http.test.ts`, `should propagate exception in sync data handler`
timed out once in the full run and passes alone in 3 s.

</details>
@robobun

robobun commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator Author

Status: #43502 (merged, 9b7c982) put these message fixes on main: ERR_HTTP_CONTENT_LENGTH_MISMATCH (client and server), ERR_HTTP_TRAILER_INVALID, ERR_STREAM_DESTROYED from ServerResponse, ERR_OPERATION_FAILED in the FileHandle writer, and ERR_SCRIPT_EXECUTION_INTERRUPTED. The one message that is still wrong on main is ERR_HTTP2_UNSUPPORTED_PROTOCOL ("gopher:" on 1.4.3-canary.1+367d939d9, Node.js v26.3.0 says protocol "gopher:" is unsupported.). #43505 is open for it, so this PR stays closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants