Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughThe change adds ChangesRequire extension loader synchronization
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Repeated assignments to require.extensions now reliably reach the loader, and new tests cover literal-key and variable-key cases. No merge-blocking risk is evident. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 6:55 AM PT - Oct 1st, 2026
❌ @robobun, your commit 06726d8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38854That installs a local version of the PR into your bun-38854 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM. The one-flag production change matches the existing ProhibitsPropertyCaching pattern in NodeVM, JSSharedEnvMap, and NodeSqlite, and the JSEnvironmentVariableMap sibling I flagged earlier is now explicitly called out as an intentional exclusion with a stated read-IC trade-off, which satisfies the fix-the-whole-class rule.
What was reviewed:
- Confirmed the flag is the same predicate every JSC put/delete caching path consults, and that three in-tree classes already use it for the same reason.
- Checked the five new tests: interpreter-cache (literal key) and JIT-IC (variable key under
BUN_JSC_useConcurrentJIT=0) paths both covered;tempDir/using/bunEnvspread/concurrent pipe drain/exit-code-last conventions followed; every test restores.jsinfinally. - Verified the header comment was trimmed to one line per the comment-cop feedback.
Extended reasoning...
Overview
The production change is a single token: JSC::ProhibitsPropertyCaching added to JSCommonJSExtensions::StructureFlags in src/jsc/bindings/JSCommonJSExtensions.h, plus a one-line comment. The rest of the diff is five new tests in the existing test/js/node/module/require-extensions.test.ts file.
The bug: JSCommonJSExtensions::put() mirrors every assignment to require.extensions[ext] into the native loader table, then tail-calls Base::put(..., slot), which fills the PutPropertySlot as a cacheable Replace. Once the LLInt/baseline/DFG cache the store for that statement, put() is bypassed and the native table goes stale. ProhibitsPropertyCaching clears propertyAccessesAreCacheable(), which is the predicate every put/delete caching path (LLInt metadata, tryCachePutBy, PutByStatus::computeFor) consults, so every store takes the slow path and put() runs every time.
Security risks
None. This tightens behavior (an override that should run now runs on every assignment). No new user-controlled input is parsed; the flag only forces the existing slow path.
Level of scrutiny
Low-to-medium. The mechanism is a well-understood JSC structure flag already used for exactly this purpose in three other classes in src/jsc/bindings/ (NodeVM.h:121, JSSharedEnvMap at JSEnvironmentVariableMap.cpp:603, NodeSqlite.h:735). require.extensions is not a hot path — the loader reads the native table, not this JS object — so the get-cache cost of the flag is irrelevant here. The alternative (slot.disableCaching()) is correctly rejected in the description because it would not cover PutByStatus::computeFor.
Other factors
- Prior review threads resolved. My earlier inline comment asked that
JSEnvironmentVariableMap(the one remainingOverridesPutsite without the flag) be either fixed or explicitly excluded. The author added a paragraph to the PR description explaining why it is intentionally excluded (ProhibitsPropertyCachingwould also disable get ICs forprocess.env.NODE_ENVreads, which that object is designed around; the fix there needs a separate decision). REVIEW.md's rule is satisfied by stating the exclusion. The comment-cop bot's objection to the multi-line header comment was also addressed — it is now one line. - Tests. Five new tests, all in the existing file. Four in-process literal-key tests pin the interpreter cache; one spawned variable-key test warmed 200× under
BUN_JSC_useConcurrentJIT=0pins the baseline/DFGput_by_valICs (the pirates/ts-node shape). All follow harness conventions:tempDirwithusing,{...bunEnv, ...},Promise.all([stdout, stderr, exited]), stderr/stdout asserted before exit code, cleanup infinallyregistered before assertions. The author verified 10 pass / 5 fail with the header reverted, 15 pass with the fix. - No CODEOWNERS cover the changed paths.
|
A second, independent report of this bug arrived. This PR already fixes it, so no other PR is open for it. The report
Verification on current main
Merge conflict
Script from the report'use strict';
const Module = require('module');
const fs = require('fs'), path = require('path'), os = require('os');
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ext-put-ic-'));
process.on('exit', () => fs.rmSync(dir, { recursive: true, force: true }));
const file = path.join(dir, 'a.js');
fs.writeFileSync(file, 'module.exports = 1;\n');
const variant = process.argv[2] || 'assign';
const native = Module._extensions['.js'];
function install(ext, loader) {
if (variant === 'define') Reflect.defineProperty(Module._extensions, ext, { value: loader, writable: true, enumerable: true, configurable: true });
else Module._extensions[ext] = loader;
}
let misses = 0, first = -1;
for (let i = 0; i < 200; i++) {
let called = false;
install('.js', function hook(mod, filename) { called = true; return native(mod, filename); });
delete require.cache[file];
require(file);
install('.js', native);
if (!called) { misses++; if (first < 0) first = i; }
}
console.log(`variant=${variant}: custom loader NOT called in ${misses}/200 iterations${first >= 0 ? ', first miss at iteration ' + first : ''}`);Run it as |
…ches the loader table JSCommonJSExtensions::put and deleteProperty mirror each assignment into the native extension table. JSObject::put marks the PutPropertySlot as cacheable, so the second assignment executed from the same statement was served by a property inline cache and never reached put(); the JS object showed the new handler while the loader kept dispatching to the old one. Setting ProhibitsPropertyCaching on the structure makes every put and delete take the slow path, the same as the shared process.env map and the node:sqlite objects.
…tion Literal-key stores are cached by the interpreter, so the existing tests fail on their second iteration. Variable-key stores (put_by_val) are only cached once the helper is JIT compiled, so this spawns a fixture that warms install/restore helpers up under BUN_JSC_useConcurrentJIT=0 and then checks that later installs and restores still reach the loader.
9b484be to
2823860
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
LGTM: a one-token structure-flag change that matches the existing in-tree precedent (NodeVM, JSSharedEnvMap), and the PR now states why the main-thread process.env sibling is deliberately left out, which closes the whole-class concern from the earlier review.
Extended reasoning...
The native change adds JSC::ProhibitsPropertyCaching to JSCommonJSExtensions::StructureFlags in src/jsc/bindings/JSCommonJSExtensions.h so put()/deleteProperty() (which mirror assignments into the native commonjs_custom_extensions table that require() actually dispatches on) run on every store instead of only the first per statement. It touches no security-sensitive surface; the only cost is slow-path property access on require.extensions itself, which the loader does not read per require. The flag's semantics are the standard JSC mechanism behind propertyAccessesAreCacheable, two sibling classes in the same directory already use it for the same reason, and the five new tests exercise interpreter and JIT caching paths with proper pipe draining and cleanup. The one inline finding is a nit about warm-up robustness, and the changed files are not covered by CODEOWNERS, so a human look is not required.
…G compiled The JIT fixture relied on the default tier-up thresholds and asserted nothing about compilation. Pin the policy with BUN_JSC_jitPolicyScale, mark the helpers noInline, and check numberOfDFGCompiles() after the warm-up loop, so the test cannot pass in the interpreter by accident.
This branch has not been deployed
1 participant
Problem
require.extensions[ext](aliasModule._extensions) only reaches the loader the first time a given statement runs. Once the statement is cached, the new handler shows up on the JS object butrequire()keeps dispatching to whatever the statement stored last time it was not cached. With a literal key (extensions[".js"] = fnin a loop) the interpreter caches the second execution already; with a variable key (theextensions[ext] = fnloop that pirates,@babel/registerand ts-node use to install and revert hooks) the cache appears once the helper has been JIT compiled, so it hits programs that install and revert hooks repeatedly..jsin a loop: iteration 1 invokes the override, iterations 2+ load the file with the builtin loader and never call it (this is the scenario from the handoff, which attributed it to a failed load; the failure is incidental, a non-throwing override misbehaves the same way).require.extensions[".js"] === originalreads true, but the second override is still what loads.jsfiles.install(ext, fn)/restore(ext, prev)helpers called ~200 times and then used for real: every later install and restore is ignored, and the handler from the warm-up iteration at which the helper got compiled is dispatched to forever (for.jsfiles too, whileModule._extensions[".js"] === originalreads true).JSCommonJSExtensions(src/jsc/bindings/JSCommonJSExtensions.h) overridesput/deletePropertyto mirror each assignment into the native extension table (onAssign->NodeModuleModule__onRequireExtensionModify), and itsputends inBase::put, which fills thePutPropertySlotas a cacheable store. JSC then caches the store for that statement (LLIntput_by_idmetadata; baseline/DFGput_by_idandput_by_valICs viatryCachePutBy; DFG's staticPutByStatus::computeFor). All of those checkStructure::propertyAccessesAreCacheable(), none of them look atOverridesPut, so the next execution of the statement writes straight into the object andputnever runs. The native table, which is whatBun__transpileFileconsults onrequire(), is left holding the previous handler.Fix
JSC::ProhibitsPropertyCachingtoJSCommonJSExtensions::StructureFlags.propertyAccessesAreCacheable()is the single predicate consulted by every put and delete caching path in JSC (LLInt, baseline/DFG ICs for bothput_by_idandput_by_val, delete ICs, andPutByStatus::computeFor, which can compile a direct store from a known structure even when no IC exists). Clearing it makes every data-property store and delete on this one object take the slow path, which is the only path that callsput/deleteProperty, so the mirroring those methods do runs on every assignment instead of only on the first one per statement. Callingslot.disableCaching()insideputwould only cover the IC paths, notPutByStatus::computeFor.JSSharedEnvMap(the SHARE_ENVprocess.env),NodeVMandNodeSqliteobjects in this tree already use the flag for the same reason.put/deletePropertyrun every time. What they do once reached is unchanged, including two pre-existing gaps that are tracked separately and are not made better or worse here:put/defineOwnPropertyupdate the native table beforeBase::put/Base::defineOwnPropertydecide whether the store is allowed (a rejected store on a frozenrequire.extensionsstill registers the handler), and an accessor descriptor passed todefinePropertyis mirrored as "unregister" because it carries no value (theModule._extensionscompat work inrequire.extensionssupport #15795 / node:module: route CJS entrypoint and CJS-via-ESM-import through Module._extensions #35774 covers that area).require.extensionsproperties take the slow path (a structure lookup). The loader itself never reads this object perrequire(), it uses the native table, so only user code touchingrequire.extensionsdirectly is affected.process.envobject (JSEnvironmentVariableMap, the one remainingOverridesPutclass insrc/jsc/bindings/without the flag) has the same bypass,process.env.X = 123run three times from one statement stores the number on the third run. It is not the same one-token fix there:process.envis built so that a variable becomes a plain data property after its first read precisely so that reads get inline cached (JSEnvironmentVariableMap.cpp, theCustomValuecomment near the bottom of the file), andProhibitsPropertyCachingwould also turn off the get caches for everyprocess.env.NODE_ENVread in the JIT tiers. The fix there has to pick between that,slot.disableCaching()input(keeps reads fast, leaves thePutByStatus::computeForpath open), or a store-only variant of the flag added in oven-sh/WebKit; it is tracked separately and nothing in this PR depends on it.test/js/node/module/require-extensions.test.ts: five new tests. Four in-process ones with literal keys (re-override of a builtin extension from a loop, a throwing override being invoked again on everyrequire, a shared restore helper, a custom extension registered three times through a helper) pin the interpreter cache; one spawned test with variable-keyinstall/restorehelpers, warmed up 200 times underBUN_JSC_useConcurrentJIT=0andBUN_JSC_jitPolicyScale=0.05, withnoInlineon both helpers and an assertion thatnumberOfDFGCompiles()is at least 1 for each before the checked rounds, pins the JIT ICs, and runs in about 0.8 s on a debug build. On a debug build of main 4b02e10 with the header reverted: 21 pass / 5 fail; with this change: 26 pass (main added 11 tests to the file since this PR was opened). The spawned test's fixture fails 10/10 on 1.4.0.test/js/node/module/,test/regression/issue/require-extensions-override.test.ts,test/regression/issue/22929-module-extensions-asi.test.ts,test/js/bun/resolve/builtin-esm-lazy-exports.test.ts,test/cli/run/run-cjs.test.ts,test/cli/run/transpiler-cache.test.ts, Node'stest-module-multi-extensions.jsandtest-require-extensions-main.js: all pass.Background
require.extensionsis a singleJSCommonJSExtensionsobject per global. The.js/.json/.ts/... properties on it are only a mirror; the tablerequire()actually dispatches on lives on the native side (commonjs_custom_extensionsinsrc/jsc/NodeModuleModule.rs, read byBun__transpileFile).JSCommonJSExtensions::put/defineOwnProperty/deletePropertyare what keeps the two in sync, so a store that skips them leaves the loader on the old handler.OverridesPutonly makes the uncached slow path call the subclass'sput; whether the result may be cached is decided by thePutPropertySlotthe slow path fills in and by the structure'sProhibitsPropertyCachingflag.put_by_idvsput_by_val:obj["lit"] = vis compiled toput_by_id, which the interpreter itself caches;obj[key] = visput_by_val, which has no interpreter cache and is only cached once the function reaches the baseline JIT. That is why the literal-key tests fail on their second iteration while the variable-key test needs a warm-up loop.PutByStatus::computeFor(StructureSet): the DFG's way of compiling a store when it already knows the receiver's structure from profiling, without going through an IC. It bails out whenpropertyAccessesAreCacheable()is false, which is why the structure flag is used rather than a per-calldisableCaching().Repro
bun 1.4.0 prints
0 true,1 false,2 false. Node, and bun with this change, printtruethree times. Unrolling the loop so each assignment is its own statement makes 1.4.0 printtruethree times as well, which is what points at the per-statement cache.The variable-key fixture in the new spawned test prints, on 1.4.0 with
BUN_JSC_useConcurrentJIT=0,warm-js-50/warm-hooked-50as the exports of every file in every round (the handlers stored by the warm-up iteration at which the helpers were compiled) together withrestored: true; with this change it printsjs-0,builtin restored-0,hooked-0, and so on.[human-review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The root cause was that
JSCommonJSExtensionslackedJSC::ProhibitsPropertyCachingin itsStructureFlags, so the JIT could inline-cache property stores on therequire.extensionsobject and repeated assignments to a key like.jsskipped the custom put path that updates the native loader table, leaving stale loaders in effect. The fix adds that flag so every assignment, whether by literal or variable key, always reaches the loader table instead of a cached fast path. New tests cover repeated overrides, restoration, custom extensions, failed loads, and a JIT-warmed assignment loop to …