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 4 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 desktop app's bundling strategy, inverting what gets bundled vs externalized and reducing asar unpacking from ~13,875 files to specific native packages. While comprehensive tests guard against the WSL-specific failure mode, the infrastructure change has platform-asymmetric behavior that warrants 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}
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.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0aacacd. Configure here.
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.
1 participant
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 desktop packaging and module resolution for the WSL backend; mis-listed externals or unpack globs fail only under WSL while Windows may still work, though tests target that closure explicitly.
Overview
Narrows Windows desktop packaging by inverting how the server CLI is bundled: most runtime JS is now inlined into the bundle instead of shipped as loose
node_modules, while only native addons, their dlopen wrappers, and Bun-only entry points stay external.Adds
scripts/lib/cli-external-packages.tsas the single source of truth for that exemption list.apps/server/vite.config.tsimportsshouldBundleCliDependencyfrom there (replacing a workspace-only prefix list), andscripts/build-desktop-artifact.tssetsWINDOWS_ASAR_UNPACKtoapps/server/dist/**plusCLI_EXTERNAL_PACKAGE_UNPACK_GLOBSderived from the same prefixes—no more**/node_modules/**.New
cli-external-packages.test.tscovers bundling rules, unpack glob alignment (including prefix siblings likenode-gyp-build-optional-packages), and a pnpm-store walk that enforces transitive runtime deps of external packages stay external (guards WSLMODULE_NOT_FOUNDwhen a dep is bundled into asar only).Reviewed by Cursor Bugbot for commit 12a5bf0. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Stop unpacking all of node_modules from the Windows asar by scoping unpacked files to external dependencies
node-pty,ffi-rs, etc.) and build-only externals (@effect/platform-bun, etc.).shouldBundleCliDependencyin vite.config.ts with the shared implementation, which now bundles most dependencies by default and only leaves the explicit external prefixes andnode:builtins unbundled.WINDOWS_ASAR_UNPACKin build-desktop-artifact.ts from a glob matching all ofnode_modulesto targeted globs derived fromCLI_EXTERNAL_PACKAGE_PREFIXES, covering both top-level and pnpm store paths.MODULE_NOT_FOUNDunder WSL).CLI_RUNTIME_EXTERNAL_PREFIXESwill now be bundled rather than left external.Macroscope summarized 12a5bf0.