perf(build): stop unpacking node_modules wholesale from the Windows asar - #5877
perf(build): stop unpacking node_modules wholesale from the Windows asar#5877tsouth89 wants to merge 8 commits into
Conversation
A Windows installer built from main writes 14,687 files, of which 13,875 are loose node_modules files under app.asar.unpacked. Only 20 of them are native .node binaries. For contrast, the entire Electron runtime -- several hundred MB -- is 22 files, because it stays inside the archive. That file count costs twice. NSIS install time tracks file count, not bytes. And every one of those files is a separate open/stat/scan the first time the server starts after an install, which is exactly when the OS file cache is cold and the on-access virus scanner is not. The blanket `**/node_modules/**` unpack exists because the CLI bundle externalizes its runtime dependencies, and the WSL backend launches plain `wsl.exe -- node`, which cannot read inside an asar. So every external dep has to be a real file on disk. Invert the bundler's rule: bundle everything except the packages that genuinely cannot be inlined -- native addons, the JS wrappers that dlopen them, and the Bun-only entry points that resolve `bun:*` specifiers -- then narrow asarUnpack to exactly that set. Measured on this tree, win/nsis x64: files written at install 14,687 -> 1,192 (-92%) loose node_modules files 13,875 -> 370 native .node binaries 20 -> 20 installer size 145.0 MiB -> 138.9 MiB Cold start improves by the same mechanism. Extracting each build's payload to a fresh directory (so the files have never been read) and booting the server: server boot to "Listening on" 9044ms / 10160ms -> 3667ms / 3779ms module load only (--version) 6521 / 6238 / 6208ms -> 761 / 659 / 654ms Run order was alternated between builds to keep cache and scanner state from favouring either one. The desktop main window is not created until the backend answers HTTP, so that ~6s comes straight off a cold launch. Both consumers now derive from one list in scripts/lib/cli-external-packages.ts. They cannot drift, and the drift is worth guarding: a package that is external but not unpacked still resolves on the Windows primary, which runs under ELECTRON_RUN_AS_NODE and reads app.asar transparently. It fails only under WSL. `node-gyp-build-optional-packages` hit exactly this while writing the patch -- matched as external by the `node-gyp-build` prefix, missed by a glob without a trailing wildcard, and invisible on the platform being tested on. Verified the way this can actually fail: extracted app.asar.unpacked into a directory with no node_modules ancestor -- what plain node sees under WSL -- and booted the server there. Migrations ran, it listened on 127.0.0.1, and no module failed to resolve. node-pty, ffi-rs, msgpackr-extract and @ff-labs/fff-node all load from that isolated tree.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
github-actions
Bot
added
vouch:unvouched
ApprovabilityVerdict: Needs human review This PR fundamentally changes the Windows build's bundling strategy, introducing new package scanning logic, build-time validation, and external package handling. While well-documented and tested, the infrastructure complexity and subtle platform-dependent failure modes warrant human review. You can customize Macroscope's approvability policy. Learn more. |
Real WSL testing on this branch found a case the hand-maintained list could not
catch by inspection.
node-gyp-build-optional-packages is external, so it is loaded from the real
filesystem, so its own `require` resolves from the real filesystem too. It
requires detect-libc, which was not on the list and therefore got bundled into
the CLI bundle -- present only inside app.asar. The Windows primary reads that
transparently under ELECTRON_RUN_AS_NODE and resolves it; plain node under WSL
cannot. msgpackr-extract failed through the same chain.
Measured under Ubuntu 24.04 with Linux node v24.18.0 against the packaged tree:
before: MISSING (cjs) msgpackr-extract [MODULE_NOT_FOUND] detect-libc
MISSING (cjs) node-gyp-build-optional-packages [MODULE_NOT_FOUND]
after : no resolution failures
The general rule is that an external package's entire runtime dependency
closure must be external. That is not something to maintain by staring at a
list, so it is now a test: it walks each runtime-external package's declared
dependencies transitively and fails if any would be bundled away.
Writing that test surfaced a distinction the single list had flattened. The
Bun-only entries are external for a build-time reason -- they resolve `bun:*`
specifiers that do not exist when bundling for Node -- and Node never loads
them, so their closure genuinely does not need to be external. The native
packages are external for a runtime reason and theirs does. The list is split
along that line, and the closure test applies only to the runtime set.
Adds 6 files to the installer (1,192 -> 1,198). Native binaries and installer
size are unchanged.
github-actions
Bot
added
size:L
|
Fair verdict, and the concern was the right one — so I went and tested it under real WSL. It found a bug. Pushed a fix in 0aacacd. What broke. Measured on Ubuntu 24.04 with Linux node v24.18.0, against the packaged tree copied out of the NSIS payload:
Why the list alone was never going to be enough. The real invariant is that an external package's entire runtime dependency closure must be external. Writing that test surfaced a distinction the single list had flattened, which I've now made explicit:
On Cost of the fix: 6 files (1,192 → 1,198). Native binaries and installer size unchanged. Happy to squash the two commits if you'd prefer a single one. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
The guard added in the previous commit could pass without checking anything.
It resolved manifests with `require("<name>/package.json")` from scripts/lib,
and swallowed resolution failures as "not installed on this platform".
Under pnpm isolation that catch swallowed nearly everything. Probed from
scripts/lib, every seed failed with MODULE_NOT_FOUND -- node-pty,
msgpackr-extract, ffi-rs, node-gyp-build, detect-libc, node-addon-api. Probed
from apps/server, only its direct dependencies resolved; the transitive
packages that actually caused the WSL breakage still did not. `exports` maps
are a second hole: @ff-labs/fff-node refuses the /package.json subpath with
ERR_PACKAGE_PATH_NOT_EXPORTED, which the same catch treated as absent.
Seeding the queue from the prefix strings was wrong for a second reason: the
filter dropped every prefix ending in "/", so "@yuuang/", "@ff-labs/" and
"@msgpackr-extract/" were never visited even where resolution worked.
Read the manifests off disk from the pnpm store instead. That is the same tree
asarUnpack globs target, it reaches transitive packages, and it is not subject
to resolution or exports semantics. Seeds now come from what is installed and
matches a prefix, so scoped prefixes are covered.
Added a guard test that fails unless node-pty, node-gyp-build-optional-packages
and detect-libc are actually found, because a closure check that reads nothing
is worse than no check -- it reports success.
Verified by mutation: removing detect-libc from the list fails with
"node-gyp-build-optional-packages -> detect-libc", the real bug. The previous
version of this test passed with detect-libc removed.
|
Confirmed, both points. Good catch — the guard was worse than useless, because it reported success. Fixed in 45b4517. Point 1 is worse than "can pass". I probed it rather than reasoning about it. From From The tell I missed at the time: when the guard first ran it reported violations only from the Point 2 confirmed. Fix. Manifests are now read off disk from the pnpm store, which is the same tree Plus a guard test that fails unless Verified by mutation, not assertion. Removing The previous version of the test passed with 39 tests pass across this file and |
The closure guard walked node_modules/.pnpm and built a node_modules path under each entry. The store also contains a regular file, lock.yaml, so that path is rooted in a file rather than a directory. Linux raises ENOTDIR from the access call; Windows quietly reports false. The test therefore passed locally and failed on CI -- itself an instance of the platform asymmetry this file exists to catch. Existence checks now treat any failure as absence.
|
Hey @tsouth89, Was having trouble launching this in WSL only mode ran it though an agent and found this line 243 in needs to be updated from: to this Seems like the rest of the file is fine and after modification of that one line everything seems to be working as expected |
The WSL health probe resolved "effect" to confirm the server's dependencies
were unpacked on the real filesystem. That premise held while the bundle
externalized its runtime deps and the whole node_modules tree was unpacked.
This branch inlines those dependencies, so "effect" no longer exists on disk.
The probe therefore exits 3 and wsl-only mode refuses to launch, reporting a
packaging regression that isn't one.
Resolve node-pty instead: it is external precisely because it cannot be
inlined, so it is a valid sentinel for the unpacked tree both before and after
this change. Verified against the packaged tree, where require.resolve("effect")
fails with MODULE_NOT_FOUND and require.resolve("node-pty/package.json")
succeeds.
Reported by @ikifar2012, who hit it running wsl-only mode from this branch.
|
Nice find, thanks. You're right about the cause: this branch inlines the server's JS deps into the bundle, so Pushed your fix in 2134d96. Used node-pty since it's external precisely because it can't be inlined, so it stays a valid sentinel either way. Also updated the two comments that still described the old behaviour, and the exit-3 message downstream. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2134d96. Configure here.
The probe now resolves node-pty rather than "effect", but the user-facing reason still named "effect" and described an unreadable bundled node_modules. That points anyone hitting a packaging failure at a package this branch deliberately inlines. Reworded to name the native packages that actually have to be unpacked.
|
Good catch, fixed in the latest push. Reworded it to name node-pty and the native packages that actually have to be unpacked, rather than effect. Checked the rest of the file and the tests for other references to the old sentinel while I was in there. The only remaining mentions of effect are in the comment explaining why the sentinel changed. |
|
🔴 blocker: As a result, transitive packages including I suggest using Verified locally at
I could not run the packaged Windows + WSL workflow on this Linux host, so this is not end-to-end WSL verification. |
…artifact `alwaysBundle` only forces packages IN. Returning false from the predicate means "no opinion", after which the default applies: a declared dependency stays external, a transitive one gets bundled. node-pty and @ff-labs/fff-node are declared dependencies of apps/server, so they stayed external and the packaging looked correct. msgpackr-extract, node-gyp-build-optional-packages and detect-libc are transitive, and were silently inlined. An inlined native loader resolves its prebuilds relative to the bundle, finds nothing, and falls back to a slower pure-JS path. No crash, no error, just a quiet loss of native acceleration. Wire the same list to `neverBundle`, which actually marks packages external. Every test to this point checked the dependency list rather than the bundle, so none of them saw it. Added findInlinedExternalPackages, which scans the emitted chunks for inlined externals, and wired it into the desktop build so a regression fails the build. It reports the module-region count as well, so "nothing inlined" is distinguishable from "the marker format changed and this scan is now blind" -- the failure mode the earlier closure guard had. Verified against the emitted bundle: msgpackr-extract is external again, and detect-libc and node-gyp-build-optional-packages are absent from it entirely. Reported by cursor bot.
|
You're right on all counts. Confirmed it against the emitted bundle before changing anything: The mechanism is that Also added the artifact check. Worth correcting something I said earlier in this PR: the On end to end: I built a Linux pty.node from source in Ubuntu 24.04, packaged an installer with So the accelerator loads from disk rather than falling back, and the WSL backend comes up on the platform this actually matters for. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
…ernals The bundle scan only asserted that external packages were absent. A build that externalized everything would pass it: source-file regions still exist, so the region count is non-zero and no external is inlined. That is the exact failure this change exists to prevent, because those packages are not covered by the unpack globs either and the WSL backend dies on ERR_MODULE_NOT_FOUND. The scan now reports every package it saw in a region, and the build asserts "effect" is among them. Every server module imports it, so it is inlined in any correctly bundled build -- 209 regions in the current one -- and its absence means the dependencies went external again. Verified against the emitted bundle: 788 regions, no external violations, effect inlined, 25 third-party packages inlined in total. Reported by macroscope.
|
Right, the check was one-directional. Fixed in 1bac125. The scan now reports every package it found in a region, and the build asserts I went looking for a general "every non-external dependency from package.json is inlined" assertion first, but a few declared deps legitimately never appear as regions once tree-shaken, so it would fail on correct builds. The sentinel avoids that without weakening the guarantee much. Verified against the emitted bundle: One thing worth mentioning from checking this: ajv's codegen emits |
|
the 🔴 remaining blocker: using only check emitted bare imports instead: every non-builtin bare import should match the intentional external list. a bundler metafile would also work. verified at
this is a guard/test blocker, not evidence that the current emitted bundle is broken. i still did not run the packaged windows + WSL flow. |
remaining validation problemwhat is the problem? The emitted-bundle guard at why is it a problem? Those ordinary dependencies are not included by the narrowed I reproduced the false negative by changing suggested fix Validate all bare imports in every emitted server chunk. Allow only:
Fail the build for every other bare import. Using the bundler metafile to inspect external modules would be even more robust if it is available. Add a regression test where Current head |
2 participants
Fixes the install-time and cold-start half of #5876.
What Changed
WINDOWS_ASAR_UNPACKwas["apps/server/dist/**", "**/node_modules/**"]. This inverts the CLI bundler's dependency rule — bundle everything except the packages that genuinely cannot be inlined — and narrowsasarUnpackto exactly that set.A package earns an exemption for one of two reasons:
.nodebinary cannot be inlined into JS and must sit on disk for both the Windows primary and the Linux Node inside WSL. The JS wrappers thatdlopenthem count too (ffi-rs,@ff-labs/fff-node,msgpackr-extract,node-gyp-build), since they resolve their binary by real filesystem path at runtime.@effect/platform-bunand@effect/sql-sqlite-bunare reached through a runtime-conditional dynamic import and resolvebun:sqlite, which does not exist when bundling for Node.Both consumers now derive from one list in
scripts/lib/cli-external-packages.ts, so they cannot drift.Why
The Windows installer writes 14,687 files, 13,875 of them loose
node_modulesfiles, to support 20 native binaries. The entire Electron runtime is 22 files because it stays inside the archive.That count costs twice: NSIS install time tracks file count, not bytes; and each file is a separate open/stat/scan the first time the server runs after an install, when the file cache is cold and the on-access scanner is not.
Measured on this repo, win/nsis x64:
node_modulesfiles.nodebinariesCold start, extracting each build's payload to a fresh directory so the files had never been read, alternating run order between builds:
Listening on--version)The main window is not created until the backend answers HTTP, so that ~6s comes off a cold launch.
Why one shared list
A package that is external but not unpacked still resolves on the Windows primary, which runs under
ELECTRON_RUN_AS_NODEand readsapp.asartransparently. It fails only under WSL. That asymmetry makes the drift invisible on the platform you are most likely to test on.node-gyp-build-optional-packageshit exactly this while I was writing the patch — matched as external by thenode-gyp-buildprefix, missed by a glob without a trailing wildcard. There are tests for the invariant.Verification
Extracted
app.asar.unpackedinto a directory with nonode_modulesancestor — what plainnodesees under WSL — and booted the server there. Migrations ran, it listened on127.0.0.1, and no module failed to resolve.node-pty,ffi-rs,msgpackr-extractand@ff-labs/fff-nodeall load from that isolated tree.scripts/build-desktop-artifact.test.ts(30) and the newscripts/lib/cli-external-packages.test.ts(7) pass.vp lintand@t3tools/servertypecheck are clean.One caveat on my verification: the build warned
No WSL node-pty prebuild provided, so I exercised the WSL module resolution path with Linux-shaped constraints rather than a real WSL launch. Happy to rerun with a Linuxpty.nodeprebuild if you want that closed before merging.UI Changes
None.
Checklist
Note
Medium Risk
Changes Windows packaging and WSL module resolution; a bundler/unpack mismatch would break the WSL backend or silently lose native acceleration, though new build-time checks mitigate that.
Overview
Inverts CLI bundling so Windows asar unpacks only native externals, cutting install file count ~92% and cold-start latency.
The server bundle now inlines JS dependencies and leaves only native addons (and their loaders/closures) external.
WINDOWS_ASAR_UNPACKdrops the blanket**/node_modules/**glob in favor of targeted globs from a shared list incli-external-packages.ts, used by both the bundler and the desktop packager so they cannot drift.Adds build-time checks that scan emitted
.mjschunks and fail if externals were inlined or if the bundle stopped inlining JS deps (sentinel:effect). Updates the WSL preflight probe to resolvenode-ptyinstead ofeffect, matching what remains unpacked.Reviewed by Cursor Bugbot for commit 1bac125. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Stop unpacking all node_modules from the Windows asar by bundling JS deps inline
neverBundle.**/node_modules/**asar unpack glob in scripts/build-desktop-artifact.ts with targeted globs covering only the server bundle and declared external packages.effect) or native externals are inlined.node-pty/package.jsoninstead ofeffect, sinceeffectis now bundled.Macroscope summarized 1bac125.