fix(nodevm): share npm modules

[sid-bruno]

Description

TBD

Notes:

• Adds a source code regex check to mimic the isolated context behaviour instead of using facades (this is being done to avoid having a few issues in existing running bruno scripts.
  • since modules would be resolved once, shared modules would cause const urlAtLoad = req.getUrl(); code like this to freeze for all other requests in the collection.
  • bru instanceof Object returns false on main because of cross-realm object (VMs), ALS facades don’t cause that, and in the shared host context they can flip it to true, so identity(===) / instanceof checks diverge from existing behaviour.
  • async work holes when things like process.nextTick() work outside the ALS span. There's a few native addons, and anything that is captured at load time with no later lookup would be at risk.
• Since static scans are being done, it can be inaccurate for extreme cases where the bruno globals are nested quite a few dependencies down, most common cases are being handled, but it's a trade off that needs to be mentioned here
• Depending on the direction after internal discussion the implementation might revert back to the AsyncLocalStorage and late bound facades that was originally implemented in fix(bruno-js): evaluate npm modules once instead of per script context (9.5 GB → 0.8 GB on a 2k-request run) #9078

Contribution Checklist:

• I've used AI significantly to create this pull request
• The pull request only addresses one issue or adds one feature.
• The pull request does not introduce any breaking changes
• I have added screenshots or gifs to help explain the change if applicable.
• I have read the contribution guidelines.
• Create an issue and link to the pull request.
• I've run the claude code review skill locally.

Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.

Publishing to New Package Managers

Please see here for more information.

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

CLI Test Results (Linux)

  1 files  223 suites   48s ⏱️
749 tests 749 ✅ 0 💤 0 ❌
751 runs  750 ✅ 1 💤 0 ❌

Results for commit 81d2f50.

♻️ This comment has been updated with latest results.

[dgyesbreghs]

@sid-bruno as requested, I ran this branch against the real 2,170-request collection from #9074 (16 top-level folders, collection-level scripts requiring @faker-js/faker/moment/nanoid before every request, heavy bru.runRequest setup chains, --sandbox=developer, sampling the runner's RSS every 5 s). Four findings, roughly in order of importance:

1. Breaking change: skipped requests no longer resolve with status: 'skipped'

This branch renames the synthesized status for skipped requests from 'skipped' to '-' (run-single-request.js ×2, run.js ×1). That shape reaches bru.runRequest() resolvers, and scripts in the wild branch on it — our collection's setup helpers do:

const response = await bru.runRequest(createPath);
if (response && response.status === 'skipped') return; // requests skipped via their pre-request script

Result: every skip-aware helper chain broke and 1,210 of 2,170 requests failed ("Setup step … returned status -"). This is independent of --shared-script-modules — it reproduces with the flag off too. Reverting just the rename locally makes the collection pass again (run in §3). If the rename is wanted for reporter display, could it stay 'skipped' on the object handed to scripts?

2. Default mode (flag off) is heavier than 4.0.0

Sharing is opt-in, so out of the box this branch keeps the per-context module evaluation: our run peaked at 20.6 GB RSS (4.0.0 peaked at 9.5 GB on the same collection; an 8 GB CI agent dies either way unless users discover the flag). RSS also shows a huge sawtooth (20.6 GB → 8 GB on a major GC), i.e. it's collectable garbage that V8 only reclaims near --max-old-space-size — the same pathology as #9074.

3. With the flag on, the static classifier defeats itself for the packages that matter

isContextSensitiveModule marks a module context-sensitive if its source (transitively, whole require tree) contains any of bru/req/res/test/expect/assert/console/… as a bare word, or any dynamic require. Measured against this collection's dependencies:

module	classification	why
moment	SENSITIVE → per-context	its own source uses res, test, console as identifiers (deprecation warnings use console.warn)
@faker-js/faker	SENSITIVE → per-context	poisoned transitively via dist/chunk-ZKNYQOPP.cjs
nanoid	shared	clean

So the two packages that caused #9074 are exactly the ones the heuristic cannot share. Minified/bundled dists make this common — almost any non-trivial package mentions console, test, assert or res somewhere in its tree. With the rename from §1 reverted locally so the collection actually passes, the flag-on run gives: 2170/2170 passed, peak 2.53 GB RSS, 345 s — functional, but ~3× the memory and ~1.7× the runtime of the original #9078 approach on the same run, almost entirely because faker/moment keep getting re-evaluated per script.

4. One leaked socket per request (pre-existing, #9079)

Both runs held one open TCP connection per executed request (~1,900 sockets open near the end) — that's the throwaway keep-alive agent issue from #9079, orthogonal to this branch but worth remembering for CI boxes with a 1,024 fd limit.

Comparison on this collection (same targets, same machine)

build	flag	result	peak runner RSS
4.0.0 (baseline)	—	2170/2170	9.5 GB
#9078 as merged+reworked → this branch	off	926/2170 (§1)	20.6 GB
this branch	--shared-script-modules	960/2170 (§1)	1.7 GB (not comparable, most requests failed early)
this branch + §1 rename reverted	--shared-script-modules	2170/2170	2.53 GB
#9078 original (ALS + facades)	—	2170/2170	0.9 GB
3.0.3	—	2170/2170	2.0 GB

Happy to re-run any variant (or a tweaked classifier) against this collection — turnaround is ~10 minutes.

[sid-bruno]

1. Breaking change: skipped requests no longer resolve with status: 'skipped'

Good catch, thanks, this is from #9080 so that's out of whatever I was changing but we'll get to it as well

2. Default mode (flag off) is heavier than 4.0.0

Hmm, this would need a deeper check what all got added into the CLI process

3. With the flag on, the static classifier defeats itself for the packages that matter

A more robust one would end up needing a lot more analysis and the async_hooks path causes identity mismatches and since we can't guarantee how or what people's scripts are doing, it's a risk I'm not sure would be smart to take.

4. One leaked socket per request

The --cache-ssl-session was specifically for this but yeah we should add in a cleanup phase after the request processing is done, thanks for catching that will review that in the next few days

[dgyesbreghs]

Thanks for the detailed follow-up!

On (3), completely your call — one clarification on what the merged #9078 approach actually risks, since "identity" covers a few different things: the facades keep typeof and method identity stable (bru.setVar === bru.setVar holds, typeof bru === 'object'), and AsyncLocalStorage only decides which script's objects a facade resolves to — scripts themselves never see facades, only npm modules do. The real identity caveat is narrower: inside an npm module, bru is not reference-equal to the raw per-script object, so exotic patterns (e.g. using bru as a Map key across executions) would notice. If the sniffing route stays, a middle ground that would rescue most real packages: treat only bru/req/res/expect/__brunoTestResults/__bruSetScope as poison words and drop console/test/assert (my sample: moment trips on res/test/console as plain identifiers, faker on a bundled chunk — happy to re-run the 2,170-request suite against any tweaked word list).

On (4): #9079 already implements exactly that cleanup phase — throwaway agents destroyed once the response is in (success, error, and before each redirect hop re-creates them, OAuth2 token agents included), keep-alive preserved on the wire, with an integration test that fails on main (6 sockets open at the 6th request) and passes with the fix. Feel free to take it as-is or fold it in here.

And whenever a build with the skip-status fix is up, say the word — the collection run takes me ~10 minutes.

[sid-bruno]

On (3), ....

fair enough

And whenever a build with the skip-status fix is up, say the word

Accidental breaking implementation from #9080, shouldn't have caused it, I wouldn't want to fix it in this PR causing a scope leak in the PR but have informed this internally for it to be addressed

[dgyesbreghs]

Makes sense — and confirmed main doesn't have the rename, so it's contained to this lineage. Standing offer stays: ping me on any iteration of this branch (or the #9080 fix) and I'll have the 2,170-request numbers back within the hour.

[dgyesbreghs]

Re-ran the 2,170-request collection against the current head (7622ec2, the AsyncLocalStorage + facade approach): 2170/2170 passed, peak runner RSS 1.08 GB — the memory regression is gone and the skip-status break is resolved. 👍

The one remaining thing this run shows is the socket side: it held ~2,178 open connections (one per request) for the whole run — i.e. #9079 is still needed on top of this. Not a blocker for this PR, just confirming the two are independent as expected. Nice work on the rework!

node-vm unit suite is also green (63/63) with the primitive-value guard you added.

[sid-bruno]

@dgyesbreghs

tests/runtime.spec.js has failures, specifically should expose each request URL to a cached module across sequential runs since we added VM level isolation for modules so that people using nested modules accessing bruno's global context would still work.

I'm still looking into ways to fix that without having to do the scan that I was doing previously

[dgyesbreghs]

Reproduced tests/runtime.spec.js › should expose each request URL to a cached module across sequential runs on the current head and dug into it — sharing my findings in case they're useful, no pressure to take any of it.

Why it fails

The failing assertion is captured() (a module doing const urlAtLoad = req.getUrl() at top level). dynamic() works because method calls late-bind through the facade; but urlAtLoad is a plain string captured once at module-load, and since the module is shared, that load only ever happens during run 1 → every later run sees run 1's URL. No facade can fix a captured primitive — the module genuinely has to be re-evaluated per run.

The old static scan solved this by detecting such modules, but (as we saw earlier) it also mis-flags faker/moment on incidental identifier matches, so they lose sharing.

An approach that avoids the scan: detect the touch at runtime

Instead of scanning source, watch whether a module actually reads a Bruno global while it is loading. The facade getter is the single choke point, so it's one extra line there plus a load-stack:

• Keep a moduleLoadStack (a frame per module currently being evaluated).
• In the context-global getter, if a load is in progress, flag every frame on the stack → transitive sensitivity, precise, zero false positives.
• After a module loads: if it was flagged, it's context-sensitive → don't put it in the shared cache; re-evaluate it per script context (cached per-run). Otherwise share it once as today.

faker/moment never read bru/req/res at load (their earlier "sensitive" verdict was purely the regex hitting res/test/console as minified identifiers), so at runtime they stay shared — while sequential-request-reader, which really does call req.getUrl() at load, gets re-evaluated per run.

Results on my 2,170-request collection

	result	peak RSS
current head (share-all)	2170/2170 but the seq-capture test fails	1.08 GB
head + this approach	2170/2170, seq-capture test passes	1.07 GB

So the correctness fix costs ~nothing in memory — faker/moment stay shared. bruno-js suite: the runtime.spec.js case goes green; the two index.spec.js failures (global identity/realm and call-site mapping for concurrent scripts) are pre-existing on this branch, unchanged by this — the realm one is separate: bru instanceof Object is false because the facade's Proxy target is created in the host realm, not the shared VM context (creating the target inside the VM context would fix that one, happy to look if useful).

Prototype branch (on top of your head, one file changed): dgyesbreghs@40f4a8e — take it, adapt it, or ignore it. Glad to iterate or re-benchmark any variant.

[sid-bruno]

@dgyesbreghs

A mark and re-evaluate is the same thing I'm going through, it does reduce the overall heap but might still be a tad bit larger than what was in 3.0.3, I'll push the implementation in a bit, should be able to get better numbers post that