Skip to content

node: errors/whatwg/promise v26 compat fixes (+11 tests) - #35487

Closed
cirospaciari wants to merge 5 commits into
claude/callback-throw-uncaughtfrom
claude/node-v26-fix-http
Closed

node: errors/whatwg/promise v26 compat fixes (+11 tests)#35487
cirospaciari wants to merge 5 commits into
claude/callback-throw-uncaughtfrom
claude/node-v26-fix-http

Conversation

@cirospaciari

Copy link
Copy Markdown
Member

Vendors 11 previously-failing Node v26.3.0 upstream tests (byte-verbatim) and fixes the runtime/harness gaps they found. Stacked on claude/callback-throw-uncaught.

Runtime fixes

process promise-rejection plumbing (BunProcess.cpp, ProcessObjectInternals.ts)

  • 'unhandledRejection' listeners were invoked straight from native code with zero JS frames on the stack, so Error.captureStackTrace(err, listener) inside a listener produced an empty CallSite array (stack[0] undefined — this crashes node's own common.mustNotCall when used as a listener). Dispatch now goes through a tiny JS trampoline that forwards to process.emit, so listeners see caller frames like node's processPromiseRejections path. The fixing line is the processObjectInternalsEmitUnhandledRejectionFromNativeCodeGenerator call in Bun__handleUnhandledRejection; listener-exception handling is unchanged (same emitter).
  • Warnings created from native context (no JS frames) had warning.stack === undefined; node always produces at least the "<name>: <message>" header. Process::emitWarning now writes that header when no stack was captured.
  • PromiseRejectionHandledWarning was emitted even when a 'rejectionHandled' listener existed; node only warns when nothing handled the event (if (!process.emit('rejectionHandled', ...)) emitWarning(...)). Reordered to match.

[nodejs.util.inspect.custom] function name (WebStreamsInspectCustom.cpp)

  • The custom-inspect method installed on URL/URLSearchParams (and web-stream) prototypes was an anonymous function; node names it [nodejs.util.inspect.custom] and test-whatwg-url-properties reads fn.name. The function is now created with that explicit name; property attributes unchanged.

internalBinding test shim (src/js/internal/test/binding.ts)

Test-harness fixes (test/js/node/test/common)

  • Flags scan bug: the --expose-gc / --expose-externalize-string branches ended the scan with break, so // Flags: --expose-gc --expose-internals never installed the expose-internals require interceptor ("Cannot find module 'internal/...'"). They now continue like every other handled flag. The five base tests whose Flags lines are affected (test-fs-filehandle, test-performance-eventloopdelay, test-crypto-dh-leak, test-trace-events-api, test-v8-flag-pool-size-0) still pass.
  • Vendored internals: adds byte-identical copies of node v26.3.0 lib/internal/errors.js, lib/internal/encoding.js, lib/internal/encoding/{single-byte,util}.js under common/nodeinternals/, plus the emulator entries they need (AggregateError primordial, uncurried %TypedArray% statics, a faithful FastBuffer shim, customInspectSymbol). The errors-family tests now run against node's real E/SystemError/AbortError/codes implementation.

Vendored tests (all byte-verbatim, all passing)

  • test-errors-aborterror
  • test-errors-systemerror-frozen-intrinsics
  • test-errors-systemerror-stackTraceLimit-{custom-setter,deleted,deleted-and-Error-sealed,has-only-a-getter,not-writable}
  • test-whatwg-url-properties
  • test-whatwg-encoding-custom-internals
  • test-promise-unhandled-error-with-reading-file
  • test-promise-unhandled-warn

Each fails on the unfixed build (module-resolution error, fn.name === 'anonymous', or the stack[0].getFileName crash respectively) and passes with this branch.

Investigated and deliberately not vendored (root causes)

  • test-http-raw-headers: request trailers are lost when the handler calls res.end() before the request body finishes — Bun emits req 'end' at dump time, before the parser has consumed the trailer section (repro in PR thread; overlaps http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432's parser-spill rework, so not fixed here).
  • test-errors-systemerror: asserts V8's exact TypeError text ("Cannot read properties of undefined"); JSC's differs.
  • test-errors-hide-stack-frames: needs Bun's native validators to hide their own frame from e.stack (node's hideStackFrames contract) — wide-blast-radius change, tracked separately.
  • test-abort-controller-any-timeout: only blocker is the TimeoutError message text ("The operation timed out." — WebKit — vs node's "The operation was aborted due to timeout"); an existing Bun test pins the WebKit wording, so changing it needs a maintainer call.
  • test-whatwg-url-custom-searchparams-constructor: node deviates from WebIDL (init == null treated as absent); Bun follows WebKit/spec ('null=').
  • http2 *-errors family, -binding, -ping, -debug, -padding-aligned, -socket-proxy, util tests: require node's native-handle architecture (mockable Http2Session/Http2Stream prototypes the JS layer routes through), async_hooks HTTP2PING resources, or node timer internals. (test-http2-binding/test-http2-respond-errors are already covered by http2: implement internalBinding('http2') for the vendored node test shim (+2 upstream tests) #34499.)
  • webstreams transfer/internal tests: need node's kTransfer worker-transfer protocol or internal/webstreams/* implementation internals; -adapters-to-streamreadable conflicts with Bun's deliberate lazy-lock design (documented in node:webstreams: +17 tests from Node v26.3.0 (30% → 76% of the suite), fix the gaps they found #32627).

The --expose-gc and --expose-externalize-string branches ended the flag
scan with break, so a later flag on the same line was never processed.
For '// Flags: --expose-gc --expose-internals' the expose-internals
require interceptor was silently skipped and the test failed with
"Cannot find module 'internal/...'". Continue like the other handled
flags (the --no-warnings comment already promises this).
… tests)

Vendor node v26.3.0 lib/internal/errors.js byte-verbatim under
common/nodeinternals/ and register it, so tests that require
E/SystemError/codes/AbortError from 'internal/errors' run against
node's real implementation. Adds the AggregateError primordial to the
emulator and gives internalBinding('util') the privateSymbols object
(arrow_message_private_symbol) the module destructures at load.

Passing: test-errors-aborterror, test-errors-systemerror-frozen-intrinsics,
and the five test-errors-systemerror-stackTraceLimit-* variants.
…test)

URL.prototype[util.inspect.custom] (and the other prototypes wired
through installInspectCustom) exposed an anonymous function; node names
the method '[nodejs.util.inspect.custom]' and user code can read
fn.name. Create the JSFunction with that explicit name instead of
relying on the symbol property key.

Passing: test-whatwg-url-properties.
…1 test)

Vendor node v26.3.0 lib/internal/encoding.js (plus its
internal/encoding/single-byte and internal/encoding/util submodules)
byte-verbatim under common/nodeinternals/, so
test-whatwg-encoding-custom-internals exercises node's real
getEncodingFromLabel table.

Support pieces: internalBinding('config') ({ hasIntl }) and
internalBinding('encoding_binding') (encodeInto/encodeUtf8String/
decodeUTF8 backed by TextEncoder/TextDecoder) in the test binding shim;
uncurried %TypedArray% static primordials, a FastBuffer shim, and
customInspectSymbol in the nodeinternals emulator.
…+2 tests)

Three node-parity fixes in the promise rejection plumbing:

- 'unhandledRejection' listeners were called straight from native code
  with no JS frames on the stack, so Error.captureStackTrace(err,
  listener) inside a listener yielded an empty CallSite array (node's
  own common.mustNotCall crashes on stack[0]). Dispatch now goes
  through a JS trampoline that forwards to process.emit, so listeners
  see caller frames like node's processPromiseRejections path.
  Listener-exception handling is unchanged (same emitter underneath).

- Warnings created while no JS is executing had stack === undefined;
  node always produces at least the '<name>: <message>' header. The
  string branch of Process::emitWarning now writes that header when no
  stack was captured.

- PromiseRejectionHandledWarning was emitted even when a
  'rejectionHandled' listener existed; node only warns when nothing
  handled the event. Reordered to match.

Passing: test-promise-unhandled-error-with-reading-file,
test-promise-unhandled-warn; the rest of the vendored
test-promise-unhandled-* / test-promises-* family still passes.
@cirospaciari

Copy link
Copy Markdown
Member Author

🤖 Repro for the test-http-raw-headers root cause noted in the description (request trailers lost when the handler responds before the request body completes). Node prints the trailer pair; Bun prints []:

const http = require('http');
const server = http.createServer((req, res) => {
  req.on('end', () => { console.log('rawTrailers:', JSON.stringify(req.rawTrailers)); server.close(); });
  req.resume();
  res.end('x f o o');   // respond immediately, before the request body finishes
});
server.listen(0, () => {
  const req = http.request({ port: server.address().port, path: '/' });
  req.addTrailers([['x-bAr', 'yOyOyOy']]);
  req.setHeader('transfer-ENCODING', 'CHUNKED');
  req.end('y b a r');
  req.on('response', r => r.resume());
});

Moving res.end() into the request 'end' handler makes Bun match Node, so the loss is specific to the respond-early path: markRequestAsDone() runs at res.end() and JS emits req 'end' at dump time, before the parser has consumed the trailer section, so takeRequestTrailers() finds nothing. Also reproduces on the #33191 CI binary. Fixing it means deferring the dumped request's 'end' until native message completion — that sits inside the parser-spill machinery #34432 is reworking, so it is left out of this PR.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. process: print Node-style warning report and hide internal frames in emitWarning #34608 - Both modify BunProcess.cpp's Process::emitWarning to make warning output match Node.js behavior when no user JS frames are present
  2. util.inspect: install [nodejs.util.inspect.custom] on web-platform prototypes #35292 - Both modify WebStreamsInspectCustom.cpp to fix [nodejs.util.inspect.custom] naming/behavior on web-platform prototypes

🤖 Generated with Claude Code

@cirospaciari

Copy link
Copy Markdown
Member Author

🤖 Coalesced into #35483 (same base, per review-load consolidation) — all 5 commits merged with a verified strict-superset check; closing.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant