internal-for-testing: make exposedInternals lazy so loading the module stays cheap - #37494
internal-for-testing: make exposedInternals lazy so loading the module stays cheap#37494robobun wants to merge 1 commit into
Conversation
Every entry of exposedInternals was a top-level require(), so loading bun:internal-for-testing evaluated all of those node internals and their dependency graphs. test/harness.ts requires the module from every test file (via test/preload.ts) just to read isASANEnabled, so on a debug build each `bun bd test <file>` paid about 0.8s of preload for modules the test never asked for; internal/assert/myers_diff alone costs most of that, because it requires internal/util/colors, which materializes process.stderr on load. Turn every entry into a getter, as the internal/child_process entry already was. Entries that assemble an object are memoized so repeated requires keep returning the same object. The consumers (the --expose-internals require interceptor in test/js/node/test/common/index.js, nodeinternals.js, dgram.test.ts, node-stream.test.js) all read entries by property access, so they are unaffected. Also drop the unused `fs` export, which eagerly loaded node:fs/promises for no consumer.
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 5:23 AM PT - Aug 11th, 2026
✅ @robobun, your commit ff8392a6918177e484c690b32fd029219724421f passed in 🧪 To try this PR locally: bunx bun-pr 37494That installs a local version of the PR into your bun-37494 --bun |
|
Status: reproduced on a debug build by timing Bot reviews so far: the CodeRabbit comment is only its rate-limit notice, and the claude review came back with no findings, so there is nothing to address from either. Waiting on CI. |
There was a problem hiding this comment.
LGTM — mechanical conversion of exposedInternals data properties to getters, with memoization for the assembled-object entries.
What was reviewed:
- Consumers (
test/js/node/test/common/index.jsinterceptor,nodeinternals.js,dgram.test.ts,node-stream.test.js) all read entries viaexposedInternals[id]property access — none spread/iterate the map, so laziness holds. - Removed
export const fshas no readers; the twoimport * as internalForTestingsites (headers.test.ts, highlighter.test.ts) don't touch.fs. - Test's
FORCE_COLORprobe is sound:bunEnvsetsFORCE_COLOR: undefinedand the test deletesNO_COLOR, sointernal/util/colorssees the env change only if evaluated after the assignment.
Extended reasoning...
Overview
This PR converts the exposedInternals map in src/js/internal-for-testing.ts from eager data properties (top-level require() calls) to lazy getters, following the shape the internal/child_process entry already had. The four entries that assemble a fresh object (internal/util, internal/event_target, internal/child_process, internal/fs/utils) are memoized in module-level let slots so repeated access returns the same object; the plain module-returning entries rely on the internal module registry's own caching. The unused export const fs = require("node:fs/promises").$data is removed. A new test file enforces that every entry is an accessor, that each returns a stable value, and (via a spawned child observing when internal/util/colors is first evaluated) that loading the module does not eagerly evaluate the exposed internals.
Security risks
None. bun:internal-for-testing is a test-only module gated behind debug builds / BUN_FEATURE_FLAG_INTERNAL_FOR_TESTING; nothing here touches user-facing runtime, auth, crypto, or parsing of untrusted input.
Level of scrutiny
Low-to-medium. This is test-infrastructure code with no production surface. The change is mechanically simple (property → getter) and preserves observable behavior for all consumers, which read entries by keyed property access. I checked every exposedInternals reference in the tree: the require interceptor in common/index.js:268-272 does exposedInternals[id] and caches per id; nodeinternals.js:431-432 reads two named keys; dgram.test.ts and node-stream.test.js index by literal key. None iterate or spread the object, so the getters fire only for the requested entries. The removed fs export has no consumers — grepped the whole tree, and the two namespace-import sites only destructure other names.
Other factors
The new test is well-constructed: Object.keys() does not invoke getters, so listing names is cheap; the identity check catches a forgotten ??= on an assembled-object entry; the FORCE_COLOR subprocess probe is a real observable proxy for "was internal/util/colors already evaluated", and I confirmed against src/js/internal/util/colors.ts that shouldColorize reads process.env.FORCE_COLOR at evaluation time. bunEnv unsets FORCE_COLOR and the test deletes NO_COLOR/NODE_DISABLE_COLORS before spawning, so the child's initial env cannot short-circuit the check. The author verified 3/4 tests fail without the src/ change, satisfying the fails-for-the-right-reason bar. The comment above exposedInternals documents the invariant and points at the enforcing test, which is exactly the kind of guard the review guide asks for when a non-obvious constraint is introduced.
What does this PR do?
exposedInternalsinsrc/js/internal-for-testing.ts(the map that servesrequire("internal/...")to vendored--expose-internalsnode tests) was built out of top-levelrequire()calls, so loadingbun:internal-for-testingevaluated all of those internals and everything they pull in.test/harness.tsrequires the module at import time from every test file in the repo (test/bunfig.tomlpreload ->preload.ts->harness.ts->detectASAN()), only to callisASANEnabled().On a debug build that is a fixed tax on every
bun bd test <file>. Timing the requires one by one (debug/ASAN build, after the modules harness.ts itself imports are already loaded):internal/assert/myers_diffis the surprising one: it requiresinternal/util/colors, whose load-timerefresh()readsprocess.stderr, which materializes the stderr stream and the tty/net/stream stack behind it.Fix: every entry of
exposedInternalsis now a getter, the shape theinternal/child_processentry already had. Entries that assemble an object rather than return a module (internal/util,internal/event_target,internal/child_process,internal/fs/utils) are memoized so repeated requires keep getting one object, as in node; the module-returning entries are already singletons through the internal module registry. The unusedexport const fs = require("node:fs/promises").$data(no consumer in the tree, eagerly loadednode:fs/promises) is removed; a named export can not be made lazy because the ESM namespace reads every export on load.Why this is the right layer: the harness needs the ASAN answer at import time (it feeds
bunEnv.ASAN_OPTIONSand the exportedisASANconstant), andprocess.config.variables.asanis deliberately0even in ASAN builds (BunProcess.cpp), soisASANEnabledis the correct probe and the module has to be loaded from the harness. The module therefore has to be cheap to load, which is also what its other exports already are ($newRustFunction/$cppbindings, lazyrequire()s inside functions). Nothing observable changes for consumers: the require interceptor intest/js/node/test/common/index.js,common/nodeinternals.js,dgram.test.tsandnode-stream.test.jsall read entries by property access, and the interceptor already caches per id.Measured on this debug build:
require("bun:internal-for-testing")in a bare process goes from ~1290 ms to ~150 ms;import("./harness")fromtest/goes from ~1.8 s to ~1.35 s (the rest of harness's import time is its own imports, unrelated to this module). On a release build the module loaded in ~14 ms before, so this is a dev-loop change only.#35567 makes some of the entries getters too, as a side effect of a much larger primordials change that depends on a WebKit bump; this is the standalone fix and covers the entries added since.
How did you verify your code works?
test/internal/internal-for-testing.test.ts:exposedInternalsentry is an accessor (a data-property entry, i.e. a new eagerrequire(), fails the test by name);bun:internal-for-testing, viarequire()(the harness path) and viaimport()(the test-file path), does not evaluate the exposed internals.internal/util/colorsdecideshasColorswhen it is evaluated, so a child process loads the module, then setsFORCE_COLOR, then requires colors:hasColorsistrueonly if colors had not been evaluated yet.Without the
src/change, 3 of the 4 tests fail (the getter check lists the 15 eager entries, and both load variants printfalse); with it, all pass.Also ran the consumers: the 51 vendored node tests that require one of the mapped ids plus the 15 that go through
nodeinternals.js(all pass, run the wayscripts/runner.node.mjsruns them), andtest/js/bun/udp/dgram.test.ts,test/js/node/stream/node-stream.test.js,test/internal/fifo.test.ts,test/internal/internal-module-blob.test.tswithbun bd test.