Conversation
The `bun -` and `bun -e` entry point is printed without the CommonJS wrapper and evaluated as a script. The same bytes run as a file are printed wrapped. Both transpiles had the same cache key, so one run was served the other's output: stdin printed nothing, or the file failed with "ReferenceError: module is not defined". `remove_cjs_module_wrapper` joins the hashed feature bools and the cache version moves from 27 to 28.
|
Status: fix and tests pushed. Both automated reviews reported nothing to change. Lints, clippy, miri and mordant are green. Buildkite CI (build 108586) is still running. Reproduced on bun 1.4.1 with a 5 KiB CommonJS |
|
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 (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughThe runtime transpiler cache key now includes ChangesTranspiler cache isolation
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change prevents CommonJS and eval-style transpilation from reusing incompatible cached output by including the wrapper-removal option in the cache key. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, fix, impact, verification, and relevant background. It does not use the exact template headings, but it provides the required change summary and verification details. Comment |
|
The automated review reported no actionable comments, so there is nothing to change in this push. The red |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted cache-key fix with a matching version bump and both-ordering coverage.
What was reviewed:
hash_for_runtime_transpiler:remove_cjs_module_wrapperappended to the bool array with the length bumped 17→18; the field exists onFeaturesand the array is hashed viabytemuck::cast_slice, so no padding/layout concern.EXPECTED_VERSION27→28 follows the file's own rule that any hash-input change bumps the constant.- New tests reuse the file's
env/temp_dir/newCacheCount/toSpawnscaffolding, pad past the 4 KiB threshold withBuffer.alloc, use.tsxto match the stdin loader, and drain stdout/stderr/exited concurrently in the stdin helper.
Extended reasoning...
Overview
The PR adds one entry (remove_cjs_module_wrapper) to the hand-maintained bool array in Features::hash_for_runtime_transpiler (src/js_parser/parser.rs), grows the array length annotation from 17 to 18, and bumps EXPECTED_VERSION from 27 to 28 in src/jsc/RuntimeTranspilerCache.rs with a doc comment in the same style as the previous version notes. Two tests are added to the existing test/cli/run/transpiler-cache.test.ts covering file→stdin and stdin→file orderings of the same CommonJS .tsx bytes, asserting correct output and the expected cache-entry deltas.
Security risks
None. This is a cache-key derivation change over parser feature flags; no user input parsing, no auth/crypto/permissions, no network or filesystem trust boundaries are touched. The only observable effect is that previously-colliding cache entries now get distinct feature hashes, and old entries are invalidated by the version bump.
Level of scrutiny
Low. The change is mechanical and mirrors the established pattern in the same function (a list of feature bools hashed as bytes). REVIEW.md's "cache keys cover every input that shapes the output" and "any change to cached/serialized output bumps the format version constant" are exactly what this PR does. The [bool; 18] explicit length means a miscount would fail to compile, and bytemuck::cast_slice::<bool, u8> keeps the hashing byte-for-byte with no alignment or uninit concerns.
Other factors
Tests are placed in the correct existing file, reuse the file's own beforeEach temp/cache setup and toSpawn/newCacheCount helpers, pad past the 4 KiB MINIMUM_CACHE_SIZE with Buffer.alloc(...).toString() per repo convention, and the stdin helper uses await using with concurrent Promise.all draining of stdout/stderr/exited. The .tsx extension is deliberately chosen so the file run and the stdin run agree on the loader portion of the key, which is the only case that could collide. CODEOWNERS does not cover any of the changed paths, and there are no prior reviews or outstanding objections on the timeline. The PR description's note about overlapping open PRs (#40838, #40971, #38683) touching the same list/constant is a rebase-ordering concern for humans landing them, not a defect in this change.
This branch has not been deployed
1 participant
Problem
bun a.tsxthenbun - < a.tsx(same CommonJS bytes, 4 KiB or more) prints nothing and exits 0. In the other orderbun a.tsxfails withReferenceError: module is not defined. Reproduces on 1.4.1 and main.-eentry point is transpiled withremove_cjs_module_wrapper(src/jsc/RuntimeTranspilerStore.rs:826), which drops the CommonJS wrapper (src/js_parser/p.rs:8209).Features::hash_for_runtime_transpiler(src/js_parser/parser.rs:355) skips the flag, so both transpiles share one cache key.Fix
remove_cjs_module_wrapperjoins the hashed feature bools, andEXPECTED_VERSIONmoves from 27 to 28 as with every earlier change to the hash inputs.--featurechange.test/cli/run/transpiler-cache.test.ts(both orders), both fail on the released binary. The rest of that file,run-eval.test.tsand regression tests 28159, 30887, 32686 pass.Background
src/jsc/RuntimeTranspilerCache.rs) keys transpiled output by a hash of the source bytes plus a features hash of the options that change the output. A features hash mismatch deletes the entry and re-transpiles.(function(exports, require, module, __filename, __dirname) {...})and the loader calls that function. The eval entry point getsmodule,exportsandrequireas globals and runs as a plain script, so the wrapper must be absent.-euse thetsxloader (src/bundler/options.rs:508), which is part of the key. Only a.tsxfile of the same bytes collides.Notes
Repro on the released binary (
cjsbig.tsxisconsole.log("ran", typeof module); module.exports = {}plus 5 KiB of comments):Both runs write the same
.pilefile name (same input hash) with the same features hash. The second run reads the first run's output. In the first order the wrapper function is evaluated and never called. In the second order the unwrapped statements run without amodulebinding.The evaluation paths are both in
evaluateCommonJSModuleOnce(src/jsc/bindings/JSCommonJSModule.cpp). The eval entry branch is selected byBun__VM__specifierIsEvalEntryPoint. The sync transpile path sets the same flag insrc/runtime/jsc_hooks.rs:2666.A
.jsfile does not collide: stdin is transpiled astsx, andParserOptions::hash_for_runtime_transpilerhashes thetsflag and the JSX options, so the.jsentry already had another features hash and was replaced, not served.Debug build sequence with the fix (
BUN_DEBUG_cache=1): file run writes, file run againrestored, stdin runMismatchedFeatureHashthen write, stdin run againrestored, file runMismatchedFeatureHashthen write.Why key the flag and not disable the cache for the eval entry: the features hash is where every other output-affecting switch is keyed, and a large piped script still benefits from a hit on a repeat run.
Related open PRs that touch the same list or the version constant:
is_macro_runtimeto the same list for the macro-mode collision. It is not included here.hash_for_runtime_transpilerlists the hashed fields by hand. An exhaustive destructure ofFeatureswould turn a missing field into a compile error. Not done here, since three open PRs add a line to this list.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/run/transpiler-cache.test.ts