[manusingh090]

Fixes: #67223

A single executable application built with "useVfs": true and plain
"assets" mounts its VFS only in the main thread, at a mount point numbered
in mount order. A worker started from a path into it, such as a worker script
bundled as an asset, finds nothing there. On Windows the failure is
especially confusing: paths under \\.\nul open the NUL device, so the loader
"finds" the file and then rejects \\.\nul\package.json as an invalid
package.json (ERR_INVALID_PACKAGE_CONFIG).

#66709 already mounts the VFS in workers when it is backed by "vfsArchive",
using the mount point reserved for --vfs-load. This does the same for
"assets":

• Layer 1 is reserved for the assets of such a SEA, so the mount point is the
  same in every thread whatever else a thread mounts. Other mounts are
  numbered from 2 in that case. The decision is recorded in C++ as an internal
  [sea_vfs_assets] option next to [sea_vfs_archive], so that a worker of a
  non-SEA process still does not load internal/vfs/sea.
• Workers call initSeaVfsInWorker() for either kind of SEA VFS.

For a VFS mounted with vfs.mount(), which a worker cannot see, the worker
now fails with a new ERR_VFS_NOT_MOUNTED instead of whatever the real file
system answers. The check only runs for a filename containing vfs, so other
workers do not load the VFS modules.

Tests:

• test/sea/test-single-executable-application-vfs.js: the fixture now
  starts a worker from a script bundled as an asset, which reads an asset and
  requires a module next to it, and checks it sees the same __dirname.
• test/parallel/test-vfs-worker-not-mounted.js: a worker started from a
  vfs.mount() path gets ERR_VFS_NOT_MOUNTED, and a real path that merely
  contains vfs still runs.

Verified locally on Linux x64 (Ubuntu 24.04, GCC 13):

• tools/test.py parallel/test-vfs* parallel/test-worker* parallel/test-bootstrap-modules sea/*
  passes (319 tests, no SEA test skipped).
• tools/test.py --worker parallel/test-bootstrap-modules passes, so a worker
  of a non-SEA process still loads no VFS module.
• Without the lib/ changes, both new tests fail: the SEA worker gets
  Cannot find module '/dev/null/vfs/1/worker.js', the Linux form of the
  reported error.
• make lint-js, make lint-md and cpplint pass on the changed files.

I could not test on Windows, where the original report comes from.

AI assistance was used; I have reviewed the change and run the tests above.

[nodejs-github-bot]

Review requested:

• @nodejs/config
• @nodejs/single-executable

[github-actions]

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

• the guide for first-time contributors
• the contribution and automation policies
• the pull request guide
• the AI use policy
• the Code of Conduct

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.

[pipobscure]

I'm not entirely sure this is the totally right approach. That being said, I'm also not entirely sure that's a bad thing. 😄

If anything I'm wondering whether useVFS should mount the SEA assets the same way --vfs-load does at layer=0.

I'll be looking at this in detail during the coming week.

[pipobscure]

Thanks for picking this up, and for the worker test in the SEA fixture: it tests exactly what #67223 is about, so please keep it.

Having looked at this in detail, I think the fix should go one step further and put the SEA VFS at layer 0, whatever serves it, instead of reserving layer 1 for assets.

Why layer 0

--vfs-load mounts a source, runs the entry point from it, and resolves modules against it. A SEA built with "useVfs": true does the same thing, except that both the source and the entry point are built into the executable: the assets (or the "vfsArchive" archive) are the source, and main is placed at the root of the mount and run from there. So useVfs is the SEA's --vfs-load, and layer 0 means "the VFS the process boots with":

Whether the SEA uses "assets" (SEAProvider) or "vfsArchive" (ZipProvider) is a provider detail. It shouldn't change the mount point. With this PR as it is, an archive SEA mounts at layer 0 and an assets SEA at layer 1.

--vfs-load doesn't make sense in a SEA at all. Its entry point can't apply (the SEA's main is fixed), and resolving modules against the SEA's files is what useVfs already does. Where it could come from, there is always a better option or a reason to refuse it:

In "execArgv", the author should use "vfsArchive".
Through --node-options, it would let whoever runs the binary change what it resolves, which is the reason it's kDisallowedInEnvvar.

In a worker's execArgv, the program can call vfs.mount() itself.
Today it also shifts the program's arguments in a SEA, because finishVfsMounts() splices the source into process.argv[1].

Once --vfs-load is rejected in every SEA, layer 0 is always free there, and the generic VFS code doesn't need to know about SEAs: no second reserved id, and user mounts are numbered from 1 everywhere.

[manusingh090]

@pipobscure Thanks for the detailed review, that makes sense. Treating useVfs as the SEA's --vfs-load is much simpler than keeping a second reserved layer.

I've updated the PR:

• The SEA VFS is now mounted at layer 0 whether it's backed by "assets" or "vfsArchive". kSeaLayer is gone, and other mounts are numbered from 1 as everywhere else.
• [sea_vfs_archive] and [sea_vfs_assets] are replaced by a single [sea_vfs].
• --vfs-load is now rejected in every SEA: --build-sea refuses it in "execArgv", and the executable rejects it at startup from --node-options or a worker's execArgv.
• I kept the worker test, and added checks that the assets SEA runs from <devNull>/vfs/0 and that --vfs-load is rejected in each of those places.

One question: I took "every SEA" literally, so --vfs-load is also rejected in a SEA built without useVfs. Is that what you meant, or should it only apply when useVfs is set?

I squashed everything into one commit. I've only tested on Linux so far, so if someone can try it on Windows (where the original issue showed up), that would be great.

[pipobscure]

One question: I took “every SEA” literally, so --vfs-load is also rejected in a SEA built without useVfs. Is that what you meant, or should it only apply when useVfs is set?

Yeah, I don't think allowing it makes sense as generally if you create an SEA, you want to control over what's run, so enabling this is like an escape-hatch which is risky.

I'll ping a few other people this week to go over this as well and give it another review as well.

[manusingh090]

One question: I took “every SEA” literally, so --vfs-load is also rejected in a SEA built without useVfs. Is that what you meant, or should it only apply when useVfs is set?

Yeah, I don't think allowing it makes sense as generally if you create an SEA, you want to control over what's run, so enabling this is like an escape-hatch which is risky.

I'll ping a few other people this week to go over this as well and give it another review as well.

Thanks, I’ll wait for the other reviews

[pipobscure]

LGTM. @mcollina let's have a look at this one on Tuesday

[pipobscure]

Can you rebase on the latest main please, then I can approve that and get it in.

[mcollina]

lgtm

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.74%. Comparing base (6c8a697) to head (77c4ffb).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #67239      +/-   ##
==========================================
+ Coverage   90.73%   90.74%   +0.01%     
==========================================
  Files         798      798              
  Lines      280076   280117      +41     
  Branches    53880    53892      +12     
==========================================
+ Hits       254123   254190      +67     
+ Misses      16259    16215      -44     
- Partials     9694     9712      +18     

Files with missing lines	Coverage Δ
lib/internal/errors.js	98.12% <100.00%> (+<0.01%)	⬆️
lib/internal/main/worker_thread.js	95.22% <100.00%> (+0.10%)	⬆️
lib/internal/vfs/file_system.js	99.64% <100.00%> (+<0.01%)	⬆️
lib/internal/vfs/sea.js	100.00% <100.00%> (ø)
lib/internal/vfs/sea_compile_cache.js	93.58% <100.00%> (ø)
lib/internal/vfs/setup.js	87.91% <100.00%> (+0.23%)	⬆️
src/node_options.cc	88.20% <100.00%> (ø)
src/node_options.h	95.83% <100.00%> (ø)
src/node_sea.cc	87.84% <100.00%> (+0.02%)	⬆️
src/node_sea.h	100.00% <ø> (ø)

... and 32 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[manusingh090]

@pipobscure Rebased on the latest main.

mcollina's compile cache change (#66768) touched the same code, so I adjusted it to fit this PR:

• sea_compile_cache.js now mounts the VFS at layer 0 when building the cache, the same as the executable does at startup.
• worker.js is added to sea-config-code-cache.json, so the code cache SEA runs the worker test too.
• The expected number of compile cache entries for the vfs fixture goes from 5 to 6, because worker.js is now one of the modules.

All SEA, VFS, worker and compile cache tests pass locally (Linux).

@mcollina the rebase made your approval stale. Could you take another look? The only changes since your review are the rebase and the compile cache adjustments above.

[pipobscure]

LGTM

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/78960/

[nodejs-github-bot]

Landed in 1b99d7c