Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 4 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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 (8)
WalkthroughThe code generator derives a generation identifier from module and code-generation inputs, emits a generated header, and stamps bundled module files. The runtime validates the stamp before dynamic module evaluation. Tests cover valid, missing, mismatched, non-final, and truncated stamps. ChangesInternal module generation
🚥 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/InternalModuleRegistry.cpp`:
- Around line 127-129: Update the marker validation loop in
InternalModuleRegistry to remove one trailing newline (either \n or \r\n), then
compare the remaining content suffix directly against the complete expected
marker instead of accepting the first matching occurrence. Add a regression case
covering an expected-marker decoy followed by a different final stamp.
🪄 Autofix
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: c9bbe42d-2729-45e0-9590-6f3b49b188b5
📒 Files selected for processing (5)
src/codegen/bundle-modules.tssrc/codegen/generate-js2native.tssrc/codegen/replacements.tssrc/jsc/bindings/InternalModuleRegistry.cpptest/js/bun/internal-module-dev-reload.test.ts
|
CI status: the failures in build 89116 are unrelated to this change. test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts is a pre-existing leak report on main (boringssl RSA keygen via node_crypto_binding, reported separately); the rest (node-dns, run-crash-handler, napi, inspect-error-leak, dlopen-duplicate-load) are known-flaky lanes that passed on retry. The new test in this PR runs on dev debug builds (it needs the hot-reload dir next to the binary) and passes there; CI builds skip it by design. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/codegen/bundle-modules.ts:401-404— WritingBUN_INTERNAL_MODULE_GENERATIONintoInternalModuleRegistry+numberOfModules.hputs it on the PCH path (viaInternalModuleRegistry.h→ZigGlobalObject.h→root-pch.h), so any change to the generation hash — e.g. adding anErrorCode.tsentry or a new$rust/$cppcall — now invalidates the PCH and forces a full C++ rebuild instead of recompiling one TU. Since the macro is only consumed insideInternalModuleRegistry.cppunder#ifdef BUN_DYNAMIC_JS_LOAD_PATH, emit it to a separate generated header (e.g.InternalModuleRegistry+generation.h) included only from that.cpp.Extended reasoning...
What the issue is
The new
#define BUN_INTERNAL_MODULE_GENERATION "<hash>"is written into the generated headerInternalModuleRegistry+numberOfModules.h. That header sits on the precompiled-header include chain:InternalModuleRegistry+numberOfModules.his included atsrc/jsc/bindings/InternalModuleRegistry.h:6InternalModuleRegistry.his included atsrc/jsc/bindings/ZigGlobalObject.h:62ZigGlobalObject.his included atsrc/jsc/bindings/root-pch.h:20, which is compiled as the PCH
The comment in
root-pch.hitself notes that editing anything reachable from it "already triggers a near-full rebuild via depfiles". Yet the only consumer ofBUN_INTERNAL_MODULE_GENERATIONis insideInternalModuleRegistry.cpp, and only inside#ifdef BUN_DYNAMIC_JS_LOAD_PATH— it has no need for header-wide (let alone PCH-wide) visibility.Why the hash is more volatile than the header's previous contents
Before this PR,
InternalModuleRegistry+numberOfModules.hcontained onlyBUN_INTERNAL_MODULE_COUNTandBUN_NATIVE_MODULE_START_INDEX, which change only when a builtin module is added or removed. The generation hash folds in significantly more:getNumericReplacementsSignature()— the full ordered list ofErrorCode.tsentries (including extra constructors) and thejs_classesordergetJS2NativeSignature()— every registered$rust/$cpp/$bindgenFncall, includingcall.filename, which forrust-type calls ispath.join(srcDir, ...)— an absolute path, so the hash also changes if the repo is checked out at a different location[moduleList, nativeStartIndex]
So while some of these inputs (js_classes) already invalidate the PCH via other generated headers, several are new PCH-invalidation triggers: appending an error code to
ErrorCode.ts, adding a$cpp(...)call in asrc/js/module, or building from a differently-located worktree.Step-by-step proof
- Developer appends one entry to the end of
src/jsc/bindings/ErrorCode.ts. No existing IDs shift. bun bdrunsbundle-modules.ts.getNumericReplacementsSignature()now returns a longererrorCtorsarray → SHA-256 input changes →generationchanges.writeIfNotChangedcompares the new content ofInternalModuleRegistry+numberOfModules.hagainst the on-disk file; theBUN_INTERNAL_MODULE_GENERATIONline differs, so it rewrites the file.- Ninja's depfile for the PCH lists this header (it is transitively included by
root-pch.h), sees its mtime changed, and rebuilds the PCH. - Every C++ TU that uses the PCH (essentially all of
src/jsc/bindings/) is now stale and recompiles — several minutes on a cold machine — for a change that, before this PR, would have touched onlyErrorCode.cppand friends.
Previously, adding a
$rustcall only touchedGeneratedJS2Native.h, which is included solely byJS2Native.cpp— a single-TU rebuild. Now it also rewrites this PCH-reachable header.Impact
This is a local-dev build-time regression only — release/CI builds don't define
BUN_DYNAMIC_JS_LOAD_PATHbut still compile the PCH, so they pay the rebuild cost too whenever the hash inputs change between commits. It doesn't affect runtime correctness. But this PR is itself about improving the dev-rebuild experience, and the fix is trivial, so it seems worth addressing here.Fix
Emit the macro to its own generated header and include it only from the one TU that uses it:
// bundle-modules.ts writeIfNotChanged( path.join(CODEGEN_DIR, "InternalModuleRegistry+generation.h"), `#define BUN_INTERNAL_MODULE_GENERATION "${generation}"\n`, );
and in
InternalModuleRegistry.cpp, inside the#ifdef BUN_DYNAMIC_JS_LOAD_PATHblock:#ifdef BUN_DYNAMIC_JS_LOAD_PATH #include "InternalModuleRegistry+generation.h" ...
Then a hash change recompiles exactly one file. (Optionally also strip
srcDirfromcall.filenameingetJS2NativeSignature()so the hash is checkout-location-independent, but that's a separate nicety.)
|
Both points addressed in 4e9c267: the generation define now lives in its own generated header (InternalModuleRegistry+generation.h) included only by InternalModuleRegistry.cpp, so a hash change recompiles one TU instead of invalidating the PCH, and the js2native signature relativizes the rust filenames so the hash no longer depends on the checkout location. The test now also gates on isDebug since codegen writes /js for release builds that never read it. |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/codegen/bundle-modules.ts:395-399— WritingBUN_INTERNAL_MODULE_GENERATIONintoInternalModuleRegistry+numberOfModules.hputs it in the PCH chain (InternalModuleRegistry.h:6→ZigGlobalObject.h:62→root-pch.h:20), so any$rust/$cpp/$bindgenFnadd/remove now triggers a full C++ rebuild instead of the previous ~1-file recompile. Also,getJS2NativeSignature()serializescall.filename, which for$rustispath.join(srcDir, ...)— an absolute repo path — so the hash (and thusnumberOfModules.hcontent) differs between checkouts, defeating theCCACHE_BASEDIR/CCACHE_NOHASHDIRpath-independence configured inscripts/build/configure.ts:202-203. Consider emitting the define into a separate header that onlyInternalModuleRegistry.cppincludes, and relativizing rust filenames in the signature.Extended reasoning...
What the bug is
The new line at
bundle-modules.ts:398writes#define BUN_INTERNAL_MODULE_GENERATION "<hash>"intoInternalModuleRegistry+numberOfModules.h. That header sits in the precompiled-header dependency chain:src/jsc/bindings/InternalModuleRegistry.h:6→#include "InternalModuleRegistry+numberOfModules.h"src/jsc/bindings/ZigGlobalObject.h:62→#include "InternalModuleRegistry.h"src/jsc/bindings/root-pch.h:20→#include "ZigGlobalObject.h"
The PCH rule in
scripts/build/compile.tsemits a depfile (-MD -MF $out.d), so ninja tracksnumberOfModules.has a PCH input. When its content changes, the PCH rebuilds and every C++ TU that consumes the PCH recompiles.Why the content now changes on common edits
The generation hash is fed by
getJS2NativeSignature()(added atgenerate-js2native.ts:178), which serializesnativeCalls.map(call => [call.id, call.type, call.filename, call.symbol]). Every$rust(...)/$cpp(...)/$bindgenFn(...)call insrc/js/**registers an entry innativeCallsin encounter order, so adding, removing, or reordering any native call changes the signature → changes the hash → changesnumberOfModules.h.writeIfNotChangeddoesn't help because the content genuinely differs.Before this PR, that same edit only rewrote
GeneratedJS2Native.h(whose sole#includeis insrc/jsc/bindings/JS2Native.cpp) plusInternalModuleRegistryConstants.h(only included byInternalModuleRegistry.cpp) — a ~1-2 TU recompile. After this PR it cascades to the entire C++ tree.Secondary issue: checkout-path-dependent hash
getJS2NativeSignature()includescall.filename. Forcall_type === "rust",resolveNativeFileId(generate-js2native.ts:118) returnspath.join(srcDir, relative)wheresrcDir = path.join(import.meta.dir, "../")— an absolute repo path. So two checkouts of the same commit at/home/a/bunand/home/b/bunproduce differentBUN_INTERNAL_MODULE_GENERATIONvalues, and therefore differentnumberOfModules.hcontent.scripts/build/configure.ts:202-203explicitly setsCCACHE_BASEDIRandCCACHE_NOHASHDIRso worktrees at different filesystem locations share.ocache entries (see the comment atscripts/build/compile.ts:126, andscripts/build/unified.ts:272which goes out of its way to keep absolute paths out of generated source for exactly this reason). This PR defeats that for every TU reached through the PCH — ccache hashes preprocessed source content, and the absolute-path-derived hash literal now appears in it.Step-by-step proof
- Developer has a debug build of commit X. They add one
$rust("foo.rs", "bar")call tosrc/js/node/foo.tsand runbun bd. bundle-modules.tspreprocesses modules;registerNativeCallpushes a new entry intonativeCalls.getJS2NativeSignature()returns a different string →generationis a different 16-hex-char value →writeIfNotChangedseesnumberOfModules.hcontent differ and rewrites it.- Ninja's depfile for the PCH lists
numberOfModules.h; its mtime changed → PCH rebuilds → everycxxrule that depends on the PCH re-runs. What was a ~1-file C++ recompile is now a full C++ rebuild.
For the ccache issue: at the same commit, worktree A at
/home/a/buncomputes a signature containing"/home/a/bun/src/runtime/node/node_os.rs"; worktree B at/home/b/buncomputes one containing"/home/b/bun/src/...". Different hashes → different#definestring literal → different preprocessed PCH source → ccache miss on every downstream TU when switching between worktrees, despiteCCACHE_BASEDIRbeing set precisely to avoid this.Impact
Neither is a runtime correctness bug — the hash is only compared against itself within one build tree, so the shipped binary is unaffected. But it's a concrete build-performance regression on a dev-loop path this PR is specifically about (editing builtin JS), and it undoes an explicitly-configured build-system property (cross-worktree ccache sharing).
Fix
The macro is only ever read inside the
#ifdef BUN_DYNAMIC_JS_LOAD_PATHblock inInternalModuleRegistry.cpp, so:- Emit
#define BUN_INTERNAL_MODULE_GENERATIONinto a separate generated header (e.g.InternalModuleRegistry+generation.h) and#includeit only fromInternalModuleRegistry.cpp, keeping it out of theInternalModuleRegistry.h→ZigGlobalObject.h→ PCH chain. - In
getJS2NativeSignature(), relativize rust filenames (e.g.path.relative(srcDir, call.filename)) so the hash is checkout-path-independent.
Both are one-liners with zero runtime effect.
|
Both findings were already addressed in 4e9c267 (this review ran against the earlier revision): the define moved to InternalModuleRegistry+generation.h, included only by InternalModuleRegistry.cpp and declared as a codegen output in f751c2b, and getJS2NativeSignature relativizes rust filenames so the hash is checkout-location-independent. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/internal-module-dev-reload.test.ts`:
- Line 79: Update the foreignStamp construction in the internal module reload
test to replace the "0".repeat(16) expression with Buffer.alloc(16,
"0").toString(), preserving the resulting 16-character zero string and newline.
- Around line 74-87: Update test/parallel-allowlist.json to add
test/js/bun/internal-module-dev-reload.test.ts to the excludeFiles list, keeping
it out of parallel execution while preserving the existing allowlist behavior
for other js/bun tests.
🪄 Autofix
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: 9d2baf06-0547-4a25-9b1e-0fa9a829fe26
📒 Files selected for processing (6)
scripts/build/codegen.tssrc/codegen/bundle-modules.tssrc/codegen/generate-js2native.tssrc/codegen/replacements.tssrc/jsc/bindings/InternalModuleRegistry.cpptest/js/bun/internal-module-dev-reload.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/parallel-allowlist.json`:
- Line 15: Regenerate test/parallel-allowlist.json using
scripts/update-parallel-allowlist.mjs with the same inputs, rather than editing
entries manually; commit the generator’s exact output so stats.files reflects
the eligible-file count and all ordering changes are applied consistently.
🪄 Autofix
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: f10d038f-ea50-495e-99ef-c1879419f0b7
📒 Files selected for processing (3)
scripts/update-parallel-allowlist.mjstest/js/bun/internal-module-dev-reload.test.tstest/parallel-allowlist.json
14669f3 to
4b50fca
Compare
|
CI status on 4b50fca (build 89134): every red lane is unrelated to this change. test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts and test/js/bun/util/filesystem_router.test.ts are pre-existing failures on main (both reported separately); the rest are known-flaky lanes that passed on retry. This change has no runtime effect on CI builds at all: the stamp check only compiles in when BUN_DYNAMIC_JS_LOAD_PATH is defined (local dev debug builds), and the new test skips without that. The diff is ready to review. |
There was a problem hiding this comment.
No issues found this pass; my earlier note about the isDebug skip gate was addressed in 4e9c267. Deferring to a human on the overall design — this introduces a new cross-language contract (codegen-written stamp ↔ C++ suffix check with _exit(1)) and touches build-infra that every debug binary depends on.
Reviewed: the hasMatchingGenerationStamp suffix scan (bounds-checked before memcmp, trailing-whitespace trim is correct); the generation hash covers all four numeric ID spaces named in the description and is checkout-location-independent; the new header is declared as a codegen output and stays off the PCH chain; the test's try/finally restores os.js on every path and the file is excluded from the parallel bucket via both the allowlist and the generator's mutatesSharedState set.
Extended reasoning...
Overview
The PR fixes a debug-build-only skew bug where hot-reloaded builtin JS from <buildDir>/js (via BUN_DYNAMIC_JS_LOAD_PATH) can dispatch $lazy(N) to the wrong native binding when an in-flight rebuild has renumbered codegen-assigned IDs. The fix hashes the four numeric ID spaces (js2native $lazy IDs, module-registry indices, $makeErrorWithCode error IDs, $inherits jsclass IDs), appends the hash as a trailing comment to every file written to the hot-reload dir, compiles the same hash into the binary via a new one-TU header, and has the debug loader _exit(1) with an actionable message on mismatch. Eight files touched: three codegen scripts, the ninja codegen output list, InternalModuleRegistry.cpp, a new test, and the parallel-allowlist plus its generator.
Security risks
None. The change is entirely debug-build developer tooling; the check and the new header include compile away when BUN_DYNAMIC_JS_LOAD_PATH is undefined (release and CI). The stamp is a build-provenance marker, not a trust boundary — the hot-reload dir is already a developer's own build tree.
Level of scrutiny
Moderate-to-high. Although the runtime effect is debug-only, the change threads through bundle-modules.ts (runs on every build for every module), adds a generated header the C++ side depends on, and introduces a new hard-exit path in InternalModuleRegistry.cpp. The design choices — stamp at EOF to preserve line numbers, hashing ID mappings not content so plain JS edits still hot-reload, a dedicated header to keep the volatile hash off the PCH include chain, _exit rather than CRASH() for a build-state error — are all reasonable and well-argued in the PR body, but they are architectural decisions on build infrastructure that a maintainer should sign off on rather than a bot.
Other factors
The PR has already been through several review rounds: my prior isDebug && hasDynamicJS gate concern, CodeRabbit's parallel-bucket exclusion, the Buffer.alloc nit, and the allowlist ordering/count are all addressed and resolved. The bug-hunting pass found nothing. I checked the C++ suffix comparison for off-by-one and unsigned-underflow (end < expectedLength guards the subtraction; end is bounded by contents.size()), confirmed getJS2NativeSignature relativizes the absolute rust filenames so the hash is stable across checkouts, confirmed the guard-rail regex @lazy\((\d+)\) matches the post-__intrinsic__→@ form in captured, and confirmed the test restores the mutated os.js in finally before assertions can leak state. The excludeFiles count (162) was independently verified by CodeRabbit against the actual array length. Given the scope — codegen infra + a new C++ failure mode — this should get a human look even though I found nothing wrong.
… generation
Non-CI debug builds load builtin JS from <buildDir>/js at runtime
(BUN_DYNAMIC_JS_LOAD_PATH) so src/js edits apply without relinking. Those
files bake in codegen-assigned numeric IDs ($lazy native-call IDs, internal
module registry indices, error-code IDs, js_classes IDs) that must match the
dispatch tables compiled into the binary. A rebuild regenerates the files
early while the old binary keeps running (and spawning children) until the
link finishes, so a renumbering change makes $lazy(N) dispatch to the wrong
native function. For node:os this surfaced as an intermittent debug assert at
module load:
[os] ASSERTION FAILED: obj[key] !== undefined
Missing freemem
at symbolToStringify (node:os:150:10)
because the os binding's $lazy ID resolved to a different module's binding
object.
bundle-modules.ts now hashes all of those ID spaces and appends the hash as a
trailing comment to every file it writes to the hot-reload dir, and compiles
the same hash into the binary. The debug loader refuses files whose stamp
does not match (or is absent, which also catches files caught mid-write) with
an error that says to finish or re-run the build. The stamp does not cover
file contents, so editing builtin JS still hot-reloads, and the embedded
sources used by release and CI builds are unchanged.
Replace the tail scan with an exact suffix match (after trimming trailing whitespace) so a matching stamp earlier in the file cannot satisfy the check when a different stamp follows it. Add a regression test for that case and trim the explanatory comments.
InternalModuleRegistry+numberOfModules.h is reachable from root-pch.h, so carrying the generation hash there would rebuild the PCH (and most of the C++) whenever an ID space changes. Emit the define into its own header included only by InternalModuleRegistry.cpp, and relativize the rust filenames in the js2native signature so the hash doesn't depend on the checkout location. Also gate the test on isDebug: codegen writes <buildDir>/js for release builds too, but only dev debug binaries read it.
getNumericReplacementsSignature duplicated the error_i numbering walk, so an edit to the rule-building loop could silently strand the signature, which is the drift class the stamp exists to catch. Serialize the numeric rules from the replacements array itself instead, matching how the js2native signature derives from the nativeCalls registry. Also assert at write time that every $lazy ID a module file bakes was registered before the stamp was computed (the stamp's completeness currently rests on statement order in bundle-modules.ts), and declare InternalModuleRegistry+generation.h as a codegen output so ninja recompiles its consumer in the same build that rewrites it.
The test mutates the build dir's hot-reload JS, so files running in parallel on the same machine could observe the tampered window. Exclude it in the allowlist and teach the generator so regeneration preserves the exclusion. Use Buffer.alloc for the repeated-string literals per test conventions.
4b50fca to
516d928
Compare
|
Rebased onto main (516d928). One conflict, in the parallel-allowlist pair: main regenerated test/parallel-allowlist.json and added its own shared-state mechanism to scripts/update-parallel-allowlist.mjs (sharedStatePrefixes). Resolved by dropping this PR's separate mutatesSharedState set and adding the dev-reload test to main's sharedStatePrefixes list instead, and by inserting the single excludeFiles entry into main's regenerated JSON. The earlier ordering-only commit became redundant and was dropped. Rebuilt and re-ran test/js/bun/internal-module-dev-reload.test.ts (4 pass, 1 skip) and test/internal/parallel-allowlist.test.ts (pass); the diff is still the same 8 files. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It introduces a new cross-cutting mechanism (generation-hash stamping between codegen and the debug C++ loader) spanning build infrastructure, codegen scripts, and InternalModuleRegistry.cpp, so a maintainer familiar with the codegen pipeline should sign off on the design.
What was reviewed:
hasMatchingGenerationStampbounds/suffix check — trims trailing whitespace, guardsend < expectedLengthbeforememcmp, correct.- Hash inputs —
getJS2NativeSignatureis called after module preprocessing registers all native calls (guard-rail check enforces this), rust filenames relativized so the hash is checkout-location-independent. - Release/CI unaffected — the check compiles out without
BUN_DYNAMIC_JS_LOAD_PATH; the generation header is included only byInternalModuleRegistry.cpp(off the PCH chain); the test gates onisDebug && fs.existsSync(...)and is excluded from the parallel bucket viasharedStatePrefixes.
Extended reasoning...
Overview
This PR fixes a debug-only developer-experience bug where a rebuild rewrites the hot-reload JS directory (<buildDir>/js) before the new binary finishes linking, causing an old bun-debug process to load JS with renumbered $lazy(N) IDs and dispatch to the wrong native binding. The fix hashes the four codegen-assigned numeric ID spaces ($lazy native-call IDs, module registry indices, $makeErrorWithCode and $inherits IDs), appends the hash as a trailing comment to every hot-reload JS file, compiles the same hash into the binary via a new InternalModuleRegistry+generation.h, and has the debug loader _exit(1) with an actionable message on mismatch.
Files touched: bundle-modules.ts (hash + stamp + guard rail), generate-js2native.ts and replacements.ts (signature helpers), InternalModuleRegistry.cpp (suffix check under #ifdef BUN_DYNAMIC_JS_LOAD_PATH), scripts/build/codegen.ts (declare the new header as a codegen output), a new test, and the parallel-allowlist pair.
Security risks
None. This is a debug-build-only guard that compares a compile-time string constant against the tail of a file the build itself wrote; release and CI binaries compile the check out entirely. No user input, network, or auth surface.
Level of scrutiny
Medium. Runtime impact is confined to local dev debug builds, but the change threads through the codegen pipeline (a new generated header, a hash whose inputs must stay in lockstep with every ID space baked into module JS, a codegen-order invariant guarded by a regex check). The design choice — hash the ID mappings rather than file contents, stamp at EOF, fatal-exit rather than fall back — is well-reasoned in the PR description, but it's a new contract between three components that a maintainer should ratify.
Other factors
The PR has been through several review rounds and every raised point is addressed and marked resolved: my earlier note on gating the test on isDebug (release build trees also get <buildDir>/js), the PCH-invalidation concern (define moved to a standalone header included only by the one .cpp), checkout-location-dependent hashing (rust filenames relativized), parallel-test isolation (added to sharedStatePrefixes and excludeFiles), and the Buffer.alloc nit. The PR body includes fails-on-main / passes-on-PR evidence showing the unfixed build reproduces the reported Missing freemem assert byte-for-byte. It was rebased on 2026-08-16 to adopt main's sharedStatePrefixes mechanism in place of the PR's earlier separate exclusion set; the author reports the test and test/internal/parallel-allowlist.test.ts pass after rebase. No outstanding unresolved comments.
This branch has not been deployed
1 participant
Symptom
Debug builds intermittently died at
node:osmodule load under concurrent process spawning:The failures looked like memory corruption (only under load, same fixture usually passed) and were first suspected to be the native
freemembinding failing under memory pressure. The binding is fine.Cause
Non-CI debug builds hot-reload builtin JS from
<buildDir>/jsat runtime (BUN_DYNAMIC_JS_LOAD_PATH) sosrc/jsedits apply without relinking. Those files bake in codegen-assigned numeric IDs:$lazy(N)native-call IDs ($cpp/$rust/$bindgenFn)$makeErrorWithCode(N, ...)error-code IDs$inherits(N, ...)js_classes IDsAll of these are assigned by codegen order, and the matching dispatch tables are compiled into the binary. A rebuild rewrites the JS files early, while the old binary keeps running and spawning children until the link finishes minutes later. If the numbering shifted,
$lazy(N)in the fresh JS dispatches to a different native function in the old binary.node:oscalls$lazy(96)for its binding; off by one it receives thenode:pathbinding, which has nofreemem, and the debug assert fires. Renumbering os.js's$lazyID by hand reproduces the reported output byte for byte, including thenode:os:150:10frame (the line number matches the on-disk dev file). The "correlates with load" observation was the rebuild itself: it pegs the machine while it opens the skew window. A build that dies after codegen leaves the skew in place until the next successful build.Fix
bundle-modules.tshashes all of the numeric ID spaces above and appends the hash as a trailing comment to every file it writes to the hot-reload dir:The same hash is compiled into the binary via
InternalModuleRegistry+numberOfModules.h, and the debug loader refuses a file whose stamp doesn't match:Design notes:
Error parsing builtin: Unexpected end of script).BUN_DYNAMIC_JS_LOAD_PATHundefined.Verification
test/js/bun/internal-module-dev-reload.test.ts(skips when the hot-reload dir doesn't exist, i.e. release, CI debug,USE_SYSTEM_BUN):$lazyID and foreign stamp is rejected with the actionable error (on an unfixed build this test fails by reproducing theMissing freememassert verbatim)Also verified by hand: normal
require('node:os')works after rebuild, all 194 files in the dir are stamped, and smoke runs oftest/js/node/os,node:path,node:util, andtest/js/web/fetch/fetch-http2-leak.test.tspass.[review] gate passed · iteration 0 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file