node builtins: defer cross-module requires and load-time work - #35541
node builtins: defer cross-module requires and load-time work#35541Jarred-Sumner wants to merge 5 commits into
Conversation
Loading builtin modules did far more work than needed at startup: eager
require() chains (node:path -> validators -> shared -> primordials,
node:tty and internal/util/colors materializing process.stderr, node:fs
loading fs/promises + the glob implementation, node:http loading the
server, node:net, node:stream umbrella and internal/util/inspect), plus
per-load work like primordials' makeSafe copies and colors' refresh().
Move those requires into the function bodies that use them, make heavy
export members (fs.promises, tty stream classes, stream classes,
http Server/globalAgent, os.constants, ...) materialize on first access,
compute primordials' Safe* entries lazily, and keep util.inspect's
implementation unloaded until first use while preserving the identity of
the public inspect function seen by custom-inspect hooks.
Also fixes a diagnostics_channel bug this exposed: WeakReference.incRef()
never held a strong reference, so a subscribed channel with no other
references could be garbage-collected together with its subscribers.
Measured in user-mode instructions (identical native code, only src/js
differs): CommonJS require("node:http") 109M -> 31M, assert 94M -> 18M,
tty 67M -> 8M, fs 41M -> 16M, util 34M -> 20M, stream 38M -> 16M,
vm 37M -> 10M, http2 116M -> 79M; importing every node builtin via ESM
drops from 245M to 222M.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| defineLazy("SafeArrayIterator", getSafeArrayIterator); | ||
| defineLazy("MapPrototypeGetSize", () => getGetter(Map, "size")); | ||
| defineLazy("SetPrototypeGetSize", () => getGetter(Set, "size")); | ||
| defineLazy("TypedArrayPrototypeGetLength", () => getGetter(Uint8Array, "length")); | ||
| defineLazy("TypedArrayPrototypeGetSymbolToStringTag", () => getGetter(Uint8Array, Symbol.toStringTag)); | ||
| defineLazy("SafeStringIterator", () => createSafeIterator(StringPrototypeSymbolIterator, StringIteratorPrototypeNext)); | ||
| defineLazy("SafeMap", () => | ||
| makeSafe( | ||
| Map, | ||
| class SafeMap extends Map { | ||
| constructor(i) { |
There was a problem hiding this comment.
🟡 Wrapping SafeMap/SafeSet/SafeWeakMap/SafeWeakSet and the *PrototypeGetSize getters in defineLazy() moves makeSafe()/getGetter()'s prototype snapshot from primordials-load to first-property-access, so a builtin loading primordials no longer immunizes the Safe* entries it doesn't touch (e.g. readable.ts's SafeSet now snapshots on the first multi-dest .pipe() rather than at module load). Bun's primordials was already load-order-dependent and none of the affected sites are security boundaries, so this is a conscious perf-vs-tamper-resistance tradeoff worth acknowledging rather than a blocker — a cheap mitigation is to capture the raw Map/Set/WeakMap/WeakSet prototype refs into module-level consts (as already done for the iterator inputs) and have the lazy makeSafe/getGetter read from those.
Extended reasoning...
What changed
makeSafe(unsafe, safe) copies own-property descriptors from unsafe.prototype at call time (Reflect.getOwnPropertyDescriptor(unsafePrototype, key)), and getGetter(cls, name) reads cls.prototype.__lookupGetter__(name) at call time. Before this PR, both ran synchronously during internal/primordials module evaluation for every Safe* class and every *PrototypeGetSize/TypedArrayPrototypeGet* getter. After this PR, each is wrapped in defineLazy(), so the snapshot for a given entry is taken on that entry's first property access instead.
Why this widens the tamper window (narrowly)
Previously, loading internal/primordials for any reason snapshotted all Safe* classes atomically. Now each entry materializes independently. Two concrete deltas:
- Lost cross-module protection.
require('node:readline')loads primordials but only touchesSafeStringIterator. Before: that also builtSafeWeakSetfrom the clean prototype. After: if user code then tampersWeakSet.prototype.hasbeforerequire('node:assert')first destructuresSafeWeakSet,makeSafecopies the tampered method. readable.ts'sSafeSetmoved from top-level destructure to insidepipe(). Before this PR theawaitDrainWritersSafeSetwas snapshotted wheninternal/streams/readableloaded; now it snapshots the first time a Readable is piped to a second destination. User code that tampersSet.prototype.addbetween those two points now poisons the "safe" set.
SafeArrayIterator/SafeStringIterator are not affected: their factory/next inputs (ArrayPrototypeSymbolIterator, ArrayIteratorPrototypeNext, StringPrototypeSymbolIterator, StringIteratorPrototypeNext) are still captured eagerly at lines 39-46, so their lazy createSafeIterator() cannot observe tampering.
Addressing the refutation
One verifier argued this is not a real regression because (a) Bun's primordials was never bootstrap-loaded, so tamper-resistance was always load-order-dependent, and (b) every consumer destructures at its own module top-level, firing the lazy getter synchronously during that same require() — so per-consumer snapshot timing is unchanged.
Point (a) is correct and important: this file was never a hard tamper boundary. Pre-PR, user code that tampered Set.prototype before requiring any primordials-consuming builtin already got a tampered SafeSet. This PR does not create a new bug class — it widens an existing conditional window. That's why this is a nit, not a blocker.
Point (b) is mostly right but not universally: readable.ts's SafeSet destructure moved out of module top-level and into pipe() in this same PR, so for that consumer the snapshot genuinely moved from module-load to first-multi-dest-pipe. And the cross-module "loading primordials for A also protects B" property — while never a documented contract — was real defense-in-depth that is now gone.
Step-by-step example
- User's entrypoint:
const { Readable } = require('stream')→ loadsinternal/streams/readable, which loadsinternal/primordials. Pre-PR:SafeSetis built here from cleanSet.prototype. Post-PR:SafeSetis not touched (readable.ts no longer top-level destructures it), so the lazy getter stays armed. - User:
Set.prototype.add = function () {}. - User:
readable.pipe(dest1); readable.pipe(dest2)→ second pipe hits thekMultiAwaitDrainbranch, doesconst { SafeSet } = require('internal/primordials')→ lazy getter fires →makeSafe(Set, ...)runs →Reflect.getOwnPropertyDescriptor(Set.prototype, 'add')returns the tamperedadd. state.awaitDrainWriters.add(dest)is now a no-op; back-pressure bookkeeping silently breaks.
Pre-PR, step 1 would have snapshotted SafeSet before step 2 could tamper it.
Why nit, not normal
- Bun's primary tamper-resistance mechanism is
$-intrinsics, not this file — the header explicitly says "TODO: Use native code and JSC intrinsics… Do not use this file for new code". - The pre-PR guarantee was already load-order-dependent; this widens an imperfect window rather than opening a new one.
- The most-used entries (
SafeMap,SafeSet, the Map/Set/TypedArray getters) are still eagerly materialized whenevernode:utilloads, viainspect_globals' top-level destructure — the PR author preserved that on purpose. - None of the affected paths (
awaitDrainWriters, assert'sSafeWeakSet, calltracker'sSafeWeakMap) are security boundaries. - The PR author explicitly acknowledged the changed guarantee (worker/messaging.ts comment updated from "at bootstrap" to "when this module loads").
Suggested cheap mitigation (optional)
Capture the raw prototypes into module-level consts alongside the iterator inputs already at lines 39-46 — e.g. const MapPrototype = Map.prototype, SetPrototype = Set.prototype, ... — and have the lazy makeSafe/getGetter closures read from those instead of the live Map/Set/WeakMap/WeakSet. That keeps all the deferred work (the ~4× Reflect.ownKeys walk + descriptor copy) lazy while restoring the pre-PR snapshot instant.
There was a problem hiding this comment.
Verified the timing: at HEAD nothing loads internal/primordials before user code either (only lazy require sites in on-demand modules; no bootstrap requireId), and a Map.prototype.size tamper on line 1 of a script is already visible to MapPrototypeGetSize on release 1.4.0 — so the snapshot was always post-user-code; this only moves it from first-module-require to first-property-access. Where a module actually depends on capturing at its own load (assert's SafeMap/SafeSet/SafeWeakSet, readline's SafeStringIterator, worker_threads/messaging's SafeMap) the destructure stays at module top level. Left as-is.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughChangesThe PR defers internal dependency loading and selected export initialization across assertion, stream, HTTP, networking, filesystem, TLS, utility, worker, and Node compatibility modules. It also updates inspection symbols, diagnostics-channel retention, TTY stream behavior, and lazy accessor wiring. Lazy initialization and Node compatibility
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/internal/fs/streams.ts`:
- Around line 6-20: Update the `loadFileHandle` destructuring assignment to bind
`fileHandlePrototypeFsync` from `FileHandle.prototype.sync` instead of `.fsync`,
preserving the `fileHandleStreamFs` fast path that compares `fh.sync`.
In `@src/js/internal/streams/compose.ts`:
- Around line 6-19: Restore the ArrayPrototypeSlice binding in the module
initialization alongside the other required internal utilities, ensuring the
existing compose logic can call it when composing multiple streams without a
ReferenceError.
In `@src/js/internal/streams/pipeline.ts`:
- Around line 277-278: Cache the lazily loaded Duplex dependency consistently in
the pipeline implementation: introduce or reuse a module-scoped lazy dependency
matching the existing addAbortListener and PassThrough patterns, and update both
Duplex require call sites—including the one around stream conversion and the
later call site—to use that cached value.
In `@src/js/internal/streams/readable.ts`:
- Line 1726: Remove the redundant comment immediately preceding the
Readable.prototype streaming operator attachment code; leave the operator
implementation unchanged.
In `@src/js/node/net.ts`:
- Around line 4199-4224: Make the lazy export and inspection paths
tamper-resistant: in src/js/node/net.ts lines 4199-4224, update both BlockList
and SocketAddress getters/setters to use captured primordial property-definition
helpers instead of mutable Object.defineProperties or Reflect.defineProperty; in
src/js/node/fs.ts lines 1420-1422, use the captured property-definition helper
in the setter; and in src/js/node/vm.ts line 331, replace the mutable Symbol.for
lookup with a captured or intrinsic symbol-registry lookup for the inspect key.
In `@src/js/node/tty.ts`:
- Around line 223-242: Update the ReadStream and WriteStream setters in the
exports object to assign the provided value to their corresponding module-level
loader cache before redefining the exported property through defineTTYValue.
Keep the getter and existing export behavior unchanged so internal loader calls
reuse the setter-assigned implementations.
- Around line 37-41: Update the lazy prototype getter in ReadStreamImpl to
preserve ReadStream’s existing constructor when replacing the prototype with one
based on fs.ReadStream.prototype. Copy the constructor from the prototype
created by $toClass before returning the lazily created prototype, so instances
continue to report ReadStream as their constructor.
In `@src/js/node/util.ts`:
- Line 20: Replace the local RegExp.prototype.exec capture with the
primordial-safe RegExpPrototypeExec imported from internal/primordials,
alongside SafeMap. Ensure styleText() hex validation continues using that
imported safe executor and remove the mutable prototype capture.
🪄 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: a3ba6715-f47d-4274-9fb8-c54487006b91
📒 Files selected for processing (64)
src/js/internal/assert/assertion_error.tssrc/js/internal/cluster/child.tssrc/js/internal/cluster/primary.tssrc/js/internal/errors.tssrc/js/internal/fs/streams.tssrc/js/internal/http/FakeSocket.tssrc/js/internal/primordials.jssrc/js/internal/promisify.tssrc/js/internal/shared.tssrc/js/internal/stream.promises.tssrc/js/internal/stream.tssrc/js/internal/streams/add-abort-signal.tssrc/js/internal/streams/compose.tssrc/js/internal/streams/destroy.tssrc/js/internal/streams/duplex.tssrc/js/internal/streams/end-of-stream.tssrc/js/internal/streams/from.tssrc/js/internal/streams/operators.tssrc/js/internal/streams/pipeline.tssrc/js/internal/streams/readable.tssrc/js/internal/streams/state.tssrc/js/internal/streams/transform.tssrc/js/internal/streams/writable.tssrc/js/internal/timers.tssrc/js/internal/tls.tssrc/js/internal/util/colors.tssrc/js/internal/util/deprecate.tssrc/js/internal/util/inspect.jssrc/js/internal/util/inspect_globals.tssrc/js/internal/validators.tssrc/js/internal/webstreams_adapters.tssrc/js/internal/worker/messaging.tssrc/js/node/_http2_upgrade.tssrc/js/node/_http_agent.tssrc/js/node/_http_client.tssrc/js/node/_http_common.tssrc/js/node/_http_incoming.tssrc/js/node/_http_outgoing.tssrc/js/node/_http_server.tssrc/js/node/assert.tssrc/js/node/child_process.tssrc/js/node/crypto.tssrc/js/node/diagnostics_channel.tssrc/js/node/fs.promises.tssrc/js/node/fs.tssrc/js/node/http.tssrc/js/node/http2.tssrc/js/node/https.tssrc/js/node/net.tssrc/js/node/os.tssrc/js/node/path.tssrc/js/node/readline.tssrc/js/node/stream.tssrc/js/node/timers.promises.tssrc/js/node/timers.tssrc/js/node/tls.tssrc/js/node/tty.tssrc/js/node/util.tssrc/js/node/v8.tssrc/js/node/vm.tssrc/js/node/wasi.tssrc/js/node/worker_threads.tssrc/js/node/zlib.tstest/js/node/diagnostics_channel/diagnostics_channel.test.ts
|
Updated 10:09 AM PT - Jul 25th, 2026
❌ @Jarred-Sumner, your commit ec4868a has some failures in 🧪 To try this PR locally: bunx bun-pr 35541That installs a local version of the PR into your bun-35541 --bun |
…-loading # Conflicts: # src/js/node/readline.ts
…er cache - pipeline: memoize the internal/streams/duplex load like the other lazy deps - internal/stream: remove the now-unused ObjectKeys binding - readable: drop a redundant comment - tty: assigning ReadStream/WriteStream also seeds the module's loader cache
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/js/node/tty.ts (1)
37-41: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLazy prototype drops
ReadStream's ownconstructor.Object.create(fs.ReadStream.prototype)discards the prototype$toClassinstalled, so instances reportfs.ReadStreamas theirconstructor. Copy theconstructoronto the new prototype.🤖 Prompt for 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. In `@src/js/node/tty.ts` around lines 37 - 41, Update the lazy prototype getter in ReadStreamImpl to preserve the constructor installed by $toClass: after creating the prototype from fs.ReadStream.prototype, copy ReadStreamImpl’s constructor onto the new prototype before returning it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/tls.ts`:
- Around line 1419-1426: Move the Object.defineProperty call for
kNativeSecureContextCtor from the Server constructor to module-scope
initialization, preserving getNativeSecureContext, enumerable: false, and
configurable: true. Ensure it is registered once on Server.prototype while
leaving constructor behavior unchanged.
In `@src/js/node/tty.ts`:
- Around line 116-117: Update the property descriptors in both class prototype
getter definitions to set enumerable to false, matching Node’s standard
non-enumerable prototype behavior. Preserve configurable: true and the existing
getter/replacement logic.
---
Duplicate comments:
In `@src/js/node/tty.ts`:
- Around line 37-41: Update the lazy prototype getter in ReadStreamImpl to
preserve the constructor installed by $toClass: after creating the prototype
from fs.ReadStream.prototype, copy ReadStreamImpl’s constructor onto the new
prototype before returning it.
🪄 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: 0441d8e8-39a4-4483-8604-d628ebe529e0
📒 Files selected for processing (8)
src/js/internal/shared.tssrc/js/internal/stream.tssrc/js/internal/streams/pipeline.tssrc/js/internal/streams/readable.tssrc/js/internal/util/inspect.jssrc/js/node/net.tssrc/js/node/tls.tssrc/js/node/tty.ts
💤 Files with no reviewable changes (2)
- src/js/internal/stream.ts
- src/js/internal/streams/readable.ts
| // Lets net.ts's SNI dispatch recognize a raw native SecureContext handed to | ||
| // an SNICallback (the `context.context || context` unwrap accepts both the | ||
| // wrapper and the unwrapped native context). | ||
| Server.prototype[kNativeSecureContextCtor] = NativeSecureContext; | ||
| Object.defineProperty(Server.prototype, kNativeSecureContextCtor, { | ||
| get: getNativeSecureContext, | ||
| enumerable: false, | ||
| configurable: true, | ||
| }); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
Hoist the kNativeSecureContextCtor accessor out of the Server constructor.
Object.defineProperty(Server.prototype, kNativeSecureContextCtor, {...}) targets the prototype and doesn't depend on this/options, yet it re-runs on every new tls.Server(...) call. Since this PR's goal is eliminating repeated per-call work, this accessor should be defined once at module scope instead of inside the constructor.
♻️ Proposed fix
function Server(options, secureConnectionListener): void {
...
- // Lets net.ts's SNI dispatch recognize a raw native SecureContext handed to
- // an SNICallback (the `context.context || context` unwrap accepts both the
- // wrapper and the unwrapped native context).
- Object.defineProperty(Server.prototype, kNativeSecureContextCtor, {
- get: getNativeSecureContext,
- enumerable: false,
- configurable: true,
- });
-
Server.prototype.getTicketKeys = function () {
throw Error("Not implented in Bun yet");
};
...
}
+
+// Lets net.ts's SNI dispatch recognize a raw native SecureContext handed to
+// an SNICallback (the `context.context || context` unwrap accepts both the
+// wrapper and the unwrapped native context). Defined once at module load
+// rather than on every Server construction.
+Object.defineProperty(Server.prototype, kNativeSecureContextCtor, {
+ get: getNativeSecureContext,
+ enumerable: false,
+ configurable: true,
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Lets net.ts's SNI dispatch recognize a raw native SecureContext handed to | |
| // an SNICallback (the `context.context || context` unwrap accepts both the | |
| // wrapper and the unwrapped native context). | |
| Server.prototype[kNativeSecureContextCtor] = NativeSecureContext; | |
| Object.defineProperty(Server.prototype, kNativeSecureContextCtor, { | |
| get: getNativeSecureContext, | |
| enumerable: false, | |
| configurable: true, | |
| }); |
🤖 Prompt for 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.
In `@src/js/node/tls.ts` around lines 1419 - 1426, Move the Object.defineProperty
call for kNativeSecureContextCtor from the Server constructor to module-scope
initialization, preserving getNativeSecureContext, enumerable: false, and
configurable: true. Ensure it is registered once on Server.prototype while
leaving constructor behavior unchanged.
| enumerable: true, | ||
| configurable: true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
enumerable: true makes prototype enumerable on both classes. Function/class prototype is non-enumerable in Node; here Object.keys(tty.ReadStream) includes "prototype" until the getter fires (after which the replacement descriptor is non-enumerable), so enumeration results differ before/after first access.
♻️ Match standard descriptor
- enumerable: true,
+ enumerable: false,
configurable: true,Also applies to: 215-216
🤖 Prompt for 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.
In `@src/js/node/tty.ts` around lines 116 - 117, Update the property descriptors
in both class prototype getter definitions to set enumerable to false, matching
Node’s standard non-enumerable prototype behavior. Preserve configurable: true
and the existing getter/replacement logic.
| let NativeSecureContext; | ||
| const getNativeSecureContext = () => (NativeSecureContext ??= $rust("SecureContext.rs", "js.getConstructor")); |
There was a problem hiding this comment.
🔴 Making NativeSecureContext lazy leaves TLSSocket.prototype[kNativeSecureContextCtor] = NativeSecureContext; (line 1003) assigning undefined at module load — the sibling Server.prototype site (line 1422) was converted to a getter but this one was missed. As a result, a socket-level SNICallback (new tls.TLSSocket(sock, { isServer: true, SNICallback })) that returns a raw native SecureContext now fails net.ts's state.server?.[kNativeSecureContextCtor] && context instanceof ... check and rejects with "Invalid SNI context". Convert line 1003 to the same Object.defineProperty(TLSSocket.prototype, kNativeSecureContextCtor, { get: getNativeSecureContext, ... }) pattern.
Extended reasoning...
What the bug is
This PR converted the eager const NativeSecureContext = $rust(...) binding in src/js/node/tls.ts into a lazy one:
let NativeSecureContext;
const getNativeSecureContext = () => (NativeSecureContext ??= $rust("SecureContext.rs", "js.getConstructor"));It correctly updated Server.prototype[kNativeSecureContextCtor] (line 1422) to an accessor that calls getNativeSecureContext, but it left the parallel assignment at line 1003 untouched:
// Lets net.ts's SNI dispatch recognize a raw native SecureContext returned by
// a socket-level SNICallback (`new tls.TLSSocket(sock, { isServer: true,
// SNICallback })`), where the handler's `this` is the socket, not a Server.
TLSSocket.prototype[kNativeSecureContextCtor] = NativeSecureContext;This line runs at module top level. Nothing between the let NativeSecureContext; declaration (line 506) and line 1003 calls getNativeSecureContext() at module scope — newNativeSecureContext (line 613) and the InternalSecureContext constructor only reference it inside function bodies. So TLSSocket.prototype[kNativeSecureContextCtor] is now permanently undefined.
The code path that triggers it
The consumer is in src/js/node/net.ts:698:
} else if (state.server?.[kNativeSecureContextCtor] && context instanceof state.server[kNativeSecureContextCtor]) {
state.selected = context;
} else {
state.failed = new Error("Invalid SNI context");
}Per the comment at tls.ts:1000-1002, when SNICallback is supplied at the socket level (new tls.TLSSocket(rawSock, { isServer: true, SNICallback })), state.server in this dispatch is the TLSSocket instance, not a tls.Server. The prototype lookup state.server[kNativeSecureContextCtor] therefore reads from TLSSocket.prototype.
Why existing code doesn't prevent it
The Server.prototype getter added at line 1422 only helps when state.server is a tls.Server. For the socket-level SNI path there is no Server, so the TLSSocket prototype is the only source of the constructor. On main (df6c7ee) NativeSecureContext was an eagerly-evaluated const, so line 1003 assigned the real constructor and this branch worked.
Step-by-step proof
require('node:tls')evaluates. Line 506:let NativeSecureContext;→undefined.- Line 1003 runs at top level:
TLSSocket.prototype[kNativeSecureContextCtor] = undefined. - User creates
const s = new tls.TLSSocket(raw, { isServer: true, SNICallback: (name, cb) => cb(null, nativeCtx) })wherenativeCtxis a raw native SecureContext (no.contextwrapper property). - During the handshake, net.ts's SNI dispatch runs with
state.server = s(the TLSSocket) andcontext = nativeCtx. context.contextisundefined(raw native handle has no JS wrapper property), soinnerContextis falsy → falls through to theelse if.state.server?.[kNativeSecureContextCtor]readss[kNativeSecureContextCtor]→TLSSocket.prototype[kNativeSecureContextCtor]→undefined. The&&short-circuits.- Falls through to
state.failed = new Error("Invalid SNI context"), and the handshake is rejected — a regression frommainwhere theinstanceofcheck would have acceptednativeCtx.
Impact and fix
This is a functional regression on a documented path (the comment at tls.ts:1000-1002 spells out exactly this use case), and it's the classic missed-sibling-site refactor called out in REVIEW.md ("Fix the whole class in the same PR — grep for every sibling site sharing the pattern"). The fix is a one-liner: replace the direct assignment at line 1003 with the same accessor pattern used at line 1422:
Object.defineProperty(TLSSocket.prototype, kNativeSecureContextCtor, {
get: getNativeSecureContext,
enumerable: false,
configurable: true,
});
| for (const key of ["defaultOptions", "replDefaults"] as const) { | ||
| Object.defineProperty(publicInspect, key, { | ||
| __proto__: null, | ||
| get() { | ||
| return loadImpl().inspect[key]; | ||
| }, | ||
| set(options) { | ||
| loadImpl().inspect[key] = options; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🟡 The replDefaults accessor on publicInspect omits configurable: true, so Object.getOwnPropertyDescriptor(require('util').inspect, 'replDefaults').configurable flips from true (pre-PR, when util.inspect was the internal inspect) to false. The internal inspect deliberately keeps replDefaults configurable (inspect.js:747) because node:repl re-defines it; Bun's own repl.js redefines it on the internal inspect so nothing internal breaks, but userland Object.defineProperty(util.inspect, 'replDefaults', ...) will now throw. Adding configurable: true to this loop's descriptor (or splitting replDefaults out) restores the previous surface.
Extended reasoning...
What the bug is
inspect_globals.ts builds publicInspect — the new function that util.ts exports as util.inspect — and mirrors defaultOptions/replDefaults onto it via a shared loop:
for (const key of ["defaultOptions", "replDefaults"] as const) {
Object.defineProperty(publicInspect, key, {
__proto__: null,
get() { return loadImpl().inspect[key]; },
set(options) { loadImpl().inspect[key] = options; },
});
}Because the descriptor omits configurable, both properties default to non-configurable. For defaultOptions this happens to match the internal inspect (inspect.js:726–735 also omits configurable). But for replDefaults it diverges: the internal inspect explicitly sets configurable: true (inspect.js:747) with a comment — "node:repl re-defines this property with its own writer-backed accessor (see REPLServer constructor); keep it configurable so that works."
The code path
Before this PR, node/util.ts did const inspect = utl.inspect where utl = require("internal/util/inspect"), so util.inspect was the internal inspect and inherited its replDefaults descriptor (configurable: true). After this PR, node/util.ts does const inspect = inspectGlobals.publicInspect, so util.inspect === publicInspect and its replDefaults descriptor is the non-configurable one from the loop above.
Why existing code doesn't prevent it
The second loop in inspect_globals.ts (for colors/styles) does set configurable: true, but the first loop does not — and replDefaults is in the first loop. Bun's own src/js/node/repl.js isn't affected because it redefines replDefaults on the internal inspect (obtained via internal/repl/node-inspect), not on util.inspect. So there's no internal breakage, only a public-surface descriptor regression.
Impact
Observable but narrow:
Object.getOwnPropertyDescriptor(require('util').inspect, 'replDefaults').configurablechanges fromtrue→false.Object.defineProperty(require('util').inspect, 'replDefaults', {...})now throwsTypeError: Attempting to change ... unconfigurable propertywhere it previously succeeded.- Plain assignment (
util.inspect.replDefaults = {...}) still works — it goes through the setter, which forwards to the internalinspect's setter and merges intoinspectReplDefaults. So the common mutation path is unaffected.
Userland calling defineProperty on util.inspect.replDefaults is exotic; nothing in Bun itself hits it. Hence nit, not blocking.
Step-by-step proof
require('node:util')runs →inspectGlobalsis loaded → thefor (const key of ["defaultOptions", "replDefaults"])loop runsObject.defineProperty(publicInspect, "replDefaults", { __proto__: null, get, set }). Per ECMA-262 §6.2.6.4/OrdinaryDefineOwnProperty, a missing[[Configurable]]in the descriptor defaults tofalsewhen the property does not already exist.util.tssetsconst inspect = inspectGlobals.publicInspectand exports it.Object.getOwnPropertyDescriptor(require('util').inspect, 'replDefaults')→{ get: [Function], set: [Function], enumerable: false, configurable: false }.- Compare pre-PR:
util.inspectwasinternal/util/inspect'sinspect, whosereplDefaultswas defined at inspect.js:736–748 with explicitconfigurable: true→ the same call returnedconfigurable: true. - Therefore
Object.defineProperty(require('util').inspect, 'replDefaults', { get() {...} })succeeded pre-PR and throws post-PR.
Fix
Add configurable: true to the first loop's descriptor (harmless for defaultOptions, required for replDefaults to match the wrapped function), or split replDefaults into its own Object.defineProperty with configurable: true to mirror inspect.js exactly.
…s ESM (#37525) Builtins implemented in `src/js` are handed to ES module importers by `generateInternalModuleSourceCode` (`src/jsc/bindings/ModuleLoader.cpp`), which snapshots the builtin's CommonJS exports object into a synthetic module record. It did that with `object->get()` on every own enumerable property, so every accessor on the exports object ran at import time. Those accessors are the builtins' lazy-loading mechanism, which means they only ever helped `require()` callers: - `node:fs`: `ReadStream`, `WriteStream`, `FileReadStream`, `FileWriteStream`, `Utf8Stream` each `require("internal/fs/streams")`, which loads the whole `node:stream` stack. - `node:tls`: `rootCertificates` parses the bundled CA store, `DEFAULT_CIPHERS` queries BoringSSL. - `node:http`: `globalAgent` instantiates the agent. `node:timers` / `node:stream`: `promises` load `timers/promises` and `stream/promises`. `node:assert`: `AssertionError` loads `internal/assert/assertion_error`. `node:repl`, `node:events`, `node:buffer`, `node:os` have a few more. #35541 is about to turn more of these into accessors (`fs.promises` among them), which this path would immediately defeat for the most common import style there is. ## Repro ```js // probe.mjs, run as `bun --expose-internals probe.mjs esm` vs `... require` const mode = process.argv[2]; const { createRequire } = await import("node:module"); let t = performance.now(); if (mode === "esm") await import("node:fs"); else createRequire(import.meta.url)("node:fs"); const fsMs = performance.now() - t; t = performance.now(); createRequire(import.meta.url)("internal/fs/streams"); console.log(mode, "node:fs", fsMs.toFixed(1), "ms; internal/fs/streams afterwards", (performance.now() - t).toFixed(1), "ms"); ``` bun 1.4.0 (release): `esm node:fs 13.3 ms; internal/fs/streams afterwards 0.2 ms` (already loaded) vs `require node:fs 7.8 ms; internal/fs/streams afterwards 5.0 ms`. On a debug build the difference is ~370ms per process, and `test/harness.ts` does `import fs from "node:fs"`, so every test file paid it. The directly observable form, which the tests use: the `node:fs` getters replace themselves with data properties when they run, so after `import fs from "node:fs"`, `Object.getOwnPropertyDescriptor(fs, "ReadStream")` had a `value` instead of a `get`. ## Fix An ES module binding cannot be made lazy from the embedder side: once the record exists, named imports read the exporting environment's slot directly (`ModuleVar`) and namespace reads go through `getValue()` in `JSModuleNamespaceObject`, so the slot has to hold the value before anything binds to it. The engine half is oven-sh/WebKit#408, merged as 7b763944f0ec. `WEBKIT_VERSION` moves from 09e477744721 (what main pins) to that sha; it is the only commit between the two: - `SyntheticModuleRecord::tryCreateWithExportNamesAndValues()` gets an overload taking a source object; an export whose value is the empty `JSValue` is declared but left in TDZ. - `materializeLazyExport()` fills such a slot from `source[name]` the first time something binds to it: `CyclicModuleRecord::initializeEnvironment` when an importer links a named import that resolves to it (directly or through `export *` / `export { x } from` chains), `JSModuleNamespaceObject::getOwnPropertySlotCommon` when a namespace read finds the slot empty (it then re-reads and installs the value the same way as before, so the namespace IC still applies), and `WebAssemblyModuleRecord::initializeImports`, which snapshots imported bindings directly (unreachable in bun today, reachable once #35587 lands). It is a no-op for a slot that already has a value, so `overrideExportValue` (what `mock.module` and `spyOn` use) keeps winning, `*namespace*` is skipped, a getter that re-enters keeps the first value that landed, and an exception from the getter propagates and leaves the slot for the next attempt. The write uses the same `symbolTablePutTouchWatchpointSet` the eager path used for its one write, so IC / DFG behaviour after materialization is identical to an eagerly built record; the baseline namespace IC already bails to the slow path on an empty slot and the DFG only folds loads that went through an installed IC. - `SyntheticSourceProvider::createWithLazyExports()` takes a generator that returns the source object, and `makeModule` passes it through. Records built any other way (JSON modules, `vm.SyntheticModule`, every existing `create()` caller) have no source object and take a pointer test on the new paths. The bun half is `generateInternalModuleSourceCode`: it now looks each property up with `getOwnPropertySlot`, snapshots data properties exactly as before, declares everything else (accessors) as a lazy export, and returns the exports object as the source. A builtin without accessors (most of them) produces a record identical to the old one. `require()` of a builtin never went through this function (`fetchCommonJSModule` returns the registry object), so it is unaffected. The engine change is inert without this opt-in: building the new WebKit with the old `ModuleLoader.cpp` still fails the new tests the same way the release does. ### Why this is the right behaviour Node builds the ESM facade of a builtin by reading the getters too, so the old behaviour was not wrong, just expensive, and there is no compat reason to keep it. The only observable difference is *when* an accessor export is sampled: at import before, at the point something first binds to it now. Data properties still snapshot at import. A namespace export or named import of an accessor is a snapshot either way (that is what `module.syncBuiltinESMExports()` exists for), and the later sample is never staler than the old one. Everything an importer can do with the export resolves to the same object `require()` would hand out, which is what the tests check for each binding path. `Object.keys(ns)`, `hasOwnProperty`, spread, and `console.log(ns)` still materialize what they touch, because `[[GetOwnProperty]]` has to produce the value; `in` ([[HasProperty]]) does not. Plugin `loader: "object"` modules and user CommonJS imported from ESM are deliberately not touched: those expose user objects whose getter timing is user-visible (#36677 is about that), whereas builtins' accessors are our own lazy-loading idiom. ## Tests `test/js/bun/resolve/builtin-esm-lazy-exports.test.ts`, next to the other module-system behaviour tests in that directory. Each case runs in its own process (a binding can only be materialized once) and checks both that importing left the `node:fs` accessors untouched and that the binding is the real class once used, for: default + named data import, named import of an accessor (exactly that one materializes), `import * as`, `import defer * as`, dynamic `import()` including `Object.keys` and that the export list is unchanged, `export *` and `export { x as y } from` re-exports (named import through the star and a read off the re-exporter's namespace, which exercises materializing on a record other than the namespace's own), accessors on `node:assert` (a function exports object), `node:timers` and `node:stream`, and `spyOn` / `mock.module` on an already imported builtin. 7 of the 8 cases fail on the current eager code (everything reports `value`); the `assert`/`timers`/`stream` identity case is a regression guard. With the fix, the probe above reports `internal/fs/streams` as not loaded after `import("node:fs")` (debug build: 338ms to load it afterwards in both modes, versus 10ms before because the import had already loaded it). Also run on the new build: `test/js/node/fs/fs.test.ts` (covers the existing `export { ReadStream, WriteStream } from "node:fs"` and `export * from "node:fs"` fixtures), the `test/js/bun/resolve` module tests, `test/js/bun/test/mock/mock-module*.test.ts`, `node-module-module.test.js`, `buffer-inspectmaxbytes.test.ts` (which pins that a named import of an accessor is a snapshot), and the events / os / timers / assert / tls-internals / node-http suites, all green apart from two failures that are identical on the released binary in this environment (`os.userInfo`, the http proxy test). <!-- robobun:evidence:begin --> --- **[decide:webkit]** gate passed · iteration 0 · 3 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 7 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/resolve/builtin-esm-lazy-exports.test.ts bun test v1.4.0 (435641a) test/js/bun/resolve/builtin-esm-lazy-exports.test.ts: 132 | afterKeys: kinds(), 133 | keysMatch: namespaceKeys.sort().join() === [...exportsKeys, "default"].sort().join(), 134 | allAreTheClasses: STREAMS.every(name => typeof ns[name] === "function" && ns[name] === fs[name]), 135 | }); 136 | `); 137 | expect(result).toEqual({ ^ error: expect(received).toEqual(expected) { "afterImport": { - "FileReadStream": "accessor", - "FileWriteStream": "accessor", - "ReadStream": "accessor", - "Utf8Stream": "accessor", - "WriteStream": "accessor", + "FileReadStream": "value", + "FileWriteStream": "value", + "ReadStream": "value", + "Utf8Stream": "value", + "WriteStream": "value", }, "afterKeys": { "FileReadStream": "value", "FileWriteStream": "value", "ReadStream": "value", "Utf8Stream": "value", "WriteStream": "value", }, "after ... (truncated) release without fix: 7 FAILED bun test v1.4.0-canary.1 (9008ae7) test/js/bun/resolve/builtin-esm-lazy-exports.test.ts: 68 | const result = await runEntry(` 69 | import { ReadStream } from "node:fs"; 70 | import { fs, kinds, print } from "./helper.mjs"; 71 | print({ kinds: kinds(), isTheClass: typeof ReadStream === "function" && ReadStream === fs.ReadStream }); 72 | `); 73 | expect(result).toEqual({ kinds: kinds("ReadStream"), isTheClass: true }); ^ error: expect(received).toEqual(expected) { "isTheClass": true, "kinds": { - "FileReadStream": "accessor", - "FileWriteStream": "accessor", + "FileReadStream": "value", + "FileWriteStream": "value", "ReadStream": "value", - "Utf8Stream": "accessor", - "WriteStream": "accessor", + "Utf8Stream": "value", + "WriteStream": "value", }, } - Expected - 4 + Received + 4 at <anonymous> (/workspace/bun/test/js/bun/resolve/builtin-esm-lazy-exports.test.ts:73:18) 89 | afterRead: kinds(), 90 | isTheClass: typeof WriteStream === "function" && WriteStream === fs.WriteStream, 91 | secondReadIsStable: ns.WriteStream === WriteStream, 92 | }) ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console 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/bun/resolve/builtin-esm-lazy-exports.test.ts bun test v1.4.0 (435641a) test/js/bun/resolve/builtin-esm-lazy-exports.test.ts: (pass) importing the module does not run its accessors [1746.65ms] (pass) a named import materializes exactly the binding it links [2145.14ms] (pass) a namespace export materializes when it is read, not on import or `in` [2175.43ms] (pass) a deferred namespace (import defer) materializes on read as well [2720.80ms] (pass) import() namespace: same export list as before, enumerating it materializes everything [2873.44ms] (pass) re-exports bind through to the builtin's own binding [2232.58ms] (pass) spyOn and mock.module on an imported builtin [2125.50ms] (pass) accessors on other builtins bind to what require() returns [2735.47ms] 8 pass 0 fail 24 expect() calls Ran 8 tests across 1 file. [8.97s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision 435641a features baseline 22 deps, 107 codegen, 1176 objects in 1540ms ninja: Entering directory `/workspace/bun/build/release' [1/1238] gen ErrorCode+*.h [2/1238] gen bindgenv2 [3/1238] gen JSBuffer.lut.h Generating /workspace/bun/build/release/codegen/JSBuffer.lut.h from /workspace/bun/src/jsc/bindings/JSBuffer.cpp [4/1238] fetch zlib [zlib] up to date [5/1238] fetch libjpeg-turbo [libjpeg-turbo] up to date [6/1238] fetch tinycc [tinycc] up to date [7/1237] fetch picohttpparser [picohttpparser] up to date [8/1237] gen .bind.ts → GeneratedBindings.cpp [9/1237] gen ProcessBindingBuffer.lut.h Generating /workspace/bun/build/release/codegen/ProcessBindingBuffer.lut.h from /workspace/bun/src/jsc/bindings/ProcessBindingBuffer.cpp [10/1237] gen ProcessBindingConstants.lut.h Generating /workspace/bun/build/release/codegen/ProcessBindingConstants.lut.h from /workspace/bun/src/jsc/bindings/ProcessBindingConstants.cpp [11/1237] gen ProcessBindingHTTPParser.lut.h Generating /workspac ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` scripts/build/deps/webkit.ts | 2 +- src/jsc/bindings/ModuleLoader.cpp | 28 ++- .../bun/resolve/builtin-esm-lazy-exports.test.ts | 232 +++++++++++++++++++++ 3 files changed, 254 insertions(+), 8 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests scripts/build/deps/webkit.ts 3 3 0 src/jsc/bindings/ModuleLoader.cpp 1 3 0 test/js/bun/resolve/builtin-esm-lazy-exports.test.ts 0 0 0 ``` </details> <!-- robobun:evidence:end -->
2 participants
What does this PR do?
Loading Node builtins did far more work than necessary at startup: eager
require()chains (node:path→ validators → shared → primordials;node:ttyandinternal/util/colorsmaterializingprocess.stderr, which builds the whole stream/fs stack;node:fs→fs/promises→ the 80KB glob impl;node:http→_http_server→node:net+ thenode:streamumbrella +internal/util/inspect), plus per-load work like primordials'makeSafeprototype copies andcolors.refresh().This defers those requires into the function bodies that use them, materializes heavy export members (
fs.promises, tty/stream classes,http.Server/globalAgent,os.constants, …) on first access, computes primordials'Safe*entries lazily, and keepsutil.inspect's implementation unloaded until first call while preserving the identity of the publicinspectfunction seen by[util.inspect.custom]hooks. Also fixes adiagnostics_channelbug this uncovered:WeakReference.incRef()never held a strong ref, so a subscribed channel with no other references could be GC'd with its subscribers.User-mode instruction counts, same native code (only
src/jsdiffers) — CommonJSrequire("node:http")109M→31M,assert94M→18M,tty67M→8M,fs41M→16M,util34M→20M,stream38M→16M,vm37M→10M,http2116M→79M; ESM-importing every node builtin 245M→222M.How did you verify your code works?
bun test test/js/nodeand the vendored Node parallel suite (test/js/node/test/parallel, ~3200 files) on a release build show the same failure set asmain; drove the debug and release binaries directly against Node for the touched surfaces; added a regression test for thediagnostics_channelGC bug (fails onmain, passes here).