install: make globalStore opt-in to preserve pnpm-compatible canonical paths#29616
install: make globalStore opt-in to preserve pnpm-compatible canonical paths#29616
Conversation
…l paths PR #29489 turned on the global virtual store by default for the isolated linker, moving materialized package files from inside the project (`<project>/node_modules/.bun/<pkg>/`) to a shared location (`<cache>/links/<pkg>-<hash>/`). Bundlers and tools that canonicalize symlinks before walking ancestors for `node_modules/` (rspack, webpack with the default resolver, many postinstall scripts) then failed to resolve optional peer / phantom / hoisted dependencies — the realpath no longer passed through the project's top-level `node_modules/`. This flips `install.globalStore` to `false` by default. The isolated linker now keeps canonical paths inside the project (the pnpm-compatible layout that the npm ecosystem assumes); the shared store is available as an explicit opt-in via `install.globalStore = true` in bunfig.toml or `BUN_INSTALL_GLOBAL_STORE=1`. Fixes #29614.
|
Updated 8:46 PM PT - Apr 22nd, 2026
❌ @robobun, your commit da3119d has 5 failures in
🧪 To try this PR locally: bunx bun-pr 29616That installs a local version of the PR into your bun-29616 --bun |
The globalStore default has never shipped in a public release (PR #29489 landed after 1.3.13), so per the repo's test-organization policy the coverage belongs alongside the other global-store tests, not in test/regression/issue/. Fold the canonical-path assertion (what bundlers actually check) into the in-place 'off by default' test and mirror the existing BUN_INSTALL_GLOBAL_STORE=0 test with a =1 opt-in variant. Drops the hand-rolled tar builder.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe global virtual store for the isolated linker is now opt-in (default false). Docs, configuration parsing, and tests were updated so Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29614.test.ts`:
- Line 157: The test currently asserts that readlinkSync(entry) contains a
platform-specific separator around "links" using `${sep}links${sep}`; change the
assertion to be separator-agnostic by matching either forward or backslash
around the literal "links" (e.g., use a regex/assertion that looks for a slash
or backslash, then "links", then a slash or backslash) when asserting on
readlinkSync(entry) so the test passes on all platforms; update the assertion
that references readlinkSync and entry accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90e9404c-362f-47a0-9bc8-db50b58d8e5f
📒 Files selected for processing (5)
docs/pm/global-store.mdxdocs/runtime/bunfig.mdxsrc/install/PackageManager/PackageManagerOptions.zigtest/cli/install/isolated-install.test.tstest/regression/issue/29614.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- test: make the BUN_INSTALL_GLOBAL_STORE=0 test prove the env override
path by first enabling the store via bunfig (otherwise with the new
default=off it vacuously passes for the wrong reason).
- test: clarify the intent of the bunfig globalStore=false test — it is
now the effective default, kept as a belt-and-suspenders pin that
explicit `false` is not coerced to `true` by a config-parsing bug.
- test: use a separator-agnostic regex (`/[\\/]links[\\/]/`) instead of
`${sep}links${sep}` so the assertion doesn't depend on slash style.
- docs: update isolated-installs.mdx to match the new opt-in default; it
previously still said the global store was on by default, contradicting
global-store.mdx and bunfig.mdx after the flip.
There was a problem hiding this comment.
No issues found and prior feedback was addressed in 3759f49 — but flipping the globalStore default back to off is a product-level tradeoff (ecosystem compat vs. the 7× warm-install win that #29489 shipped as default-on), so I'd like a maintainer to sign off on the direction rather than auto-approving.
Extended reasoning...
Overview
This PR flips install.globalStore from default-on to default-off (opt-in), reverting the default introduced by #29489 to fix #29614 (bundlers that canonicalize symlinks lose sight of the project's node_modules/). The actual source change is two lines in PackageManagerOptions.zig (the Enable.global_virtual_store field initializer and a val.len > 0 guard on the env-var parse); everything else is docs (global-store.mdx, isolated-installs.mdx, bunfig.mdx) and test updates in isolated-install.test.ts to explicitly pass globalStore: true where the shared-store layout is being exercised, plus new tests pinning the off-by-default behavior and the env-var opt-in.
Security risks
None. No auth, crypto, network, or untrusted-input handling is touched. The env-var parsing change is a strict tightening (empty string now stays off instead of toggling on).
Level of scrutiny
Code-wise this is mechanical and low-risk — a boolean flip plus consistent doc/test updates. However, it changes the default install layout for every isolated-linker user, deliberately reversing a recently-shipped default. That is a product/UX tradeoff (pnpm-compatible canonical paths vs. the warm-install speedup) that a maintainer should explicitly ratify rather than have a bot approve. Once the direction is agreed, the implementation itself needs no further scrutiny.
Other factors
My two prior findings (vacuous "disable" tests and stale wording in isolated-installs.mdx) and CodeRabbit's separator nit were all addressed in 3759f49. The robobun CI comment shows build failures on 468ee98 across every platform in scripts/build/ci.ts, which looks like an infra hiccup unrelated to this diff, but CI on 3759f49 should be confirmed green before merge.
test/js/bun/net/socket.test.ts's 'should not leak memory' asserts TCPSocket count <= 3 on Windows, but the Windows 11 ARM64 runner consistently sees 4. This is the same pre-existing flake that was already diagnosed and widened-to-4 on a separate branch (commit bd347a4: 'socket leak: widen Windows TCPSocket threshold by one (FIXME)') but that fix hasn't landed on main yet. The failing test is in src/js/bun/net/ and is completely unrelated to this PR's package-manager layout change — there is no code path in #29616 that touches TCPSocket, memory, or GC. Retrying on a different VM.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Fix the underlying issue. Why are we re-materializing? Why are we copying to .tmp folders? Let's fix the underlying issue instead of hacking around it by disabling.
| /// `<project>/node_modules/`, which breaks bundlers (rspack, webpack) | ||
| /// and tools that canonicalize symlinks before walking ancestors for | ||
| /// `node_modules/` directories. See #29614. | ||
| global_virtual_store: bool = false, |
There was a problem hiding this comment.
🔴 Flipping the default to false doesn't detach pre-existing global-store symlinks: with global_store_path == null, isolated_install.zig:1581 short-circuits has_stale_gvs_link to false (skipping the lstat), and the existsZ at :1620 follows the leftover symlink into <cache>/links/ so needs_install = false and the entry is skipped. A user upgrading from a #29489-era bun therefore still sees readlink -f node_modules/<pkg> escape into <cache>/links/ — exactly #29614 — until they rm -rf node_modules. Drop the global_store_path == null guard at :1581 (the lstat is cheap and appendLocalStoreEntryPath doesn't depend on it), and add an on→off test mirroring "upgrades a pre-global-store node_modules in place".
Extended reasoning...
What the bug is
This PR flips enable.global_virtual_store to default false so that fresh isolated installs keep canonical paths in-project. But it does not migrate a node_modules that was populated under the previous default-on behavior. The stale-link detector at isolated_install.zig:1581 short-circuits when global_store_path == null, so leftover node_modules/.bun/<storepath> → <cache>/links/... symlinks survive a plain bun install after upgrading. The result is exactly the #29614 symptom this PR is meant to fix, just on the upgrade-in-place path instead of the fresh-install path.
The specific code path
Walk through a user who installed under a #29489-era bun (GVS default-on), upgrades to a build with this PR, and runs bun install without deleting node_modules:
PackageManagerOptions.zig:756—enable.global_virtual_store = false(new default).isolated_install.zig:873—global_store_path = nullbecauseenable.global_virtual_storeis false.isolated_install.zig:~1272—node_modules/.bunalready exists, somkdiratfails andis_new_bun_modules = false.Installer.zig:1606—entryUsesGlobalStore()returnsfalsebecauseglobal_store_path == null→uses_global_store = false.isolated_install.zig:1580-1592—has_stale_gvs_link:!uses_global_storeis true, but the inner block hitsif (installer.global_store_path == null) break :stale false;at :1581 and yieldsfalse. The lstat at :1588 that would have detected the leftover symlink never runs.isolated_install.zig:1614—appendRealStorePath(sinceentryUsesGlobalStoreis false) builds the project-local pathnode_modules/.bun/<storepath>/node_modules/<pkg>/package.json.isolated_install.zig:1620—sys.existsZ(POSIXaccess(), WindowsGetFileAttributesW) traverses the stale symlink atnode_modules/.bun/<storepath>into<cache>/links/..., findspackage.json, returns true →needs_install = false.isolated_install.zig:1636-1657—!needs_installand!uses_global_store→ the entry is marked.done/.skippedand the loop continues. The detach logic in thelink_packagetask body never runs.
Why existing code doesn't prevent it
The :1581 guard was added under the assumption "if global_store_path is null, the store was never on, so no stale GVS link can exist." The comment at :1570-1579 only describes the eligibility-loss case (newly patched/trusted while the store stays enabled). The default flip in this PR invalidates that assumption: every user who installed under #29489's default-on behavior now lands in the global_store_path == null branch with stale links on disk. appendLocalStoreEntryPath (Installer.zig:~1667) does not depend on global_store_path, so the guard is purely an optimization, not a correctness requirement.
Step-by-step proof
Concrete scenario with a single dep no-deps@1.0.0:
| step | state |
|---|---|
pre-PR bun install (GVS on) |
node_modules/.bun/no-deps@1.0.0 is a symlink → <cache>/links/no-deps@1.0.0-<hash> |
| upgrade bun to this PR | filesystem unchanged |
bun install (no flags, no bunfig change) |
global_store_path = null; is_new_bun_modules = false; uses_global_store = false; has_stale_gvs_link = false (short-circuited at :1581); existsZ("node_modules/.bun/no-deps@1.0.0/node_modules/no-deps/package.json") follows the symlink and returns true; needs_install = false → skipped |
| result | readlink -f node_modules/no-deps → <cache>/links/no-deps@1.0.0-<hash>/node_modules/no-deps — outside the project |
So bundlers/resolvers that canonicalize before walking ancestors still miss the project's top-level node_modules/.
Impact
For users upgrading from a #29489-era bun (where GVS was default-on), this PR's stated fix for #29614 is silently a no-op until they rm -rf node_modules. The :1581 guard itself pre-dates this PR — explicit globalStore=true → false had the same gap — but the default flip turns it from an obscure config-change edge case into the common upgrade path for every existing user, which is why it should be addressed here rather than deferred.
Test coverage gap
No test exercises GVS on→off with a populated node_modules:
- "upgrades a pre-global-store node_modules in place" covers off→on (real dir → symlink).
- "an entry that loses global-store eligibility detaches" runs with
globalStore: true, soglobal_store_path != nulland the :1581 guard doesn't fire.
How to fix
Drop the if (installer.global_store_path == null) break :stale false; short-circuit at isolated_install.zig:1581. The lstat is cheap, only runs when !is_new_bun_modules, and correctly detects the stale link regardless of whether GVS is currently enabled. Add a test mirroring the existing off→on upgrade test: install with BUN_INSTALL_GLOBAL_STORE=1, assert the entry is a symlink, then re-install with the env var unset and assert the entry is now a real directory whose realpath stays in-project.
| if (env.get("BUN_INSTALL_GLOBAL_STORE")) |val| { | ||
| this.enable.global_virtual_store = !strings.eqlComptime(val, "0"); | ||
| // Treat any non-empty value other than "0" as opt-in. Default is off; | ||
| // BUN_INSTALL_GLOBAL_STORE=1 enables it, =0 (or unset) keeps it off. | ||
| this.enable.global_virtual_store = val.len > 0 and !strings.eqlComptime(val, "0"); |
There was a problem hiding this comment.
🟡 nit: the "(or unset) keeps it off" parenthetical is slightly inaccurate — when the env var is unset this block is skipped entirely, so a bunfig globalStore = true (lines ~296-298) persists and the store stays on. Consider "=0 forces it off; unset leaves the bunfig/default value untouched" to avoid implying unset overrides bunfig.
Extended reasoning...
What the issue is
The new comment on the BUN_INSTALL_GLOBAL_STORE handler reads:
Treat any non-empty value other than "0" as opt-in. Default is off; BUN_INSTALL_GLOBAL_STORE=1 enables it, =0 (or unset) keeps it off.
The "(or unset)" parenthetical is imprecise. When the env var is unset, env.get("BUN_INSTALL_GLOBAL_STORE") returns null, the entire if block is skipped, and this.enable.global_virtual_store retains whatever config.global_store set ~100 lines earlier (PackageManagerOptions.zig:296-298). So with globalStore = true in bunfig and the env var unset, the store stays on — "(or unset) keeps it off" is not strictly true. It is also a little odd structurally to describe the unset case from inside a block that only executes when the var is set.
Step-by-step
Enable.global_virtual_storedefaults tofalse(line 756).load()reads bunfig:if (config.global_store) |gs| { this.enable.global_virtual_store = gs; }(lines ~296-298). With bunfigglobalStore = true, the field is nowtrue.env.get("BUN_INSTALL_GLOBAL_STORE")returnsnull(unset). The wholeifat line 401 is skipped.- Result:
global_virtual_store == true. The store is on, contradicting "(or unset) keeps it off".
The comment is correct for the no-bunfig case (default off → unset keeps it off) but reads as a precedence statement covering all cases.
Why existing code doesn't prevent it
The actual behavior is correct and already pinned by the new test BUN_INSTALL_GLOBAL_STORE=0 overrides a bunfig-enabled store, whose inline comment ("PackageManagerOptions.load reads BUN_INSTALL_GLOBAL_STORE after the bunfig values, so env wins") documents the real precedence. Only this inline code comment is imprecise.
Addressing the refutation
One verifier argued the leading "Default is off;" frames the rest of the sentence as describing env-var semantics relative to the built-in default, in which frame "(or unset) keeps it off" is accurate, and that a maintainer wanting the bunfig-vs-env story can see the config.global_store block ~100 lines earlier. That is a fair charitable reading and is why this is filed as a nit, not a bug — the runtime behavior is correct, the precedence is obvious from the code structure, and the comment's primary purpose (explaining why val.len > 0 and !eql("0") is used) is served. The wording is still objectively imprecise in the bunfig=true case, the comment is new in this PR, and the fix is a two-word tweak, so it seems worth surfacing without insisting on it.
Impact
No functional impact. A future maintainer reading this comment in isolation could come away thinking unset env always means store-off (i.e., that bunfig can't enable it without the env var), which is wrong. Minor doc-comment accuracy issue.
Suggested fix
// Treat any non-empty value other than "0" as opt-in. =1 enables it,
// =0 forces it off; unset leaves the bunfig/default value untouched.
|
Investigating. What I've found so far: Symptom 2 (re-materialization) root cause: I can reproduce a smaller version (15 project-local out of 937 = 1.6%) with a real dep tree containing vite/webpack/esbuild. The reporter's 20% ratio at 10k packages fits if their graph has several trusted deps near the root of the dep graph. But this is re-materialization to Symptom 1 (canonical path outside project) is architectural: with What fix would you prefer?
I'll hold off on pushing more until I hear which direction you want. |
2 participants
Fixes #29614.
Repro
The realpath is outside
<project>/. Bundlers (rspack, webpack with the default resolver) and tools that canonicalize symlinks before walking ancestors fornode_modules/then can't see the project's top-levelnode_modules/, so they miss optional-peer / phantom / hoisted deps they used to resolve.pnpm's virtual store lives inside the project (
<project>/node_modules/.pnpm/) so canonical paths stay in-project. That's the layout the npm ecosystem assumes.Cause
PR #29489 added the global virtual store and made it default-on. The speedup is real but the behavior change (realpath now points outside the project) breaks any tooling built against the pre-#29489 layout.
Fix
Flip
install.globalStoretofalseby default. The isolated linker materializes packages inside<project>/node_modules/.bun/<storepath>/(pnpm-compatible). The shared store is available as an explicit opt-in viainstall.globalStore = trueorBUN_INSTALL_GLOBAL_STORE=1for projects that want the warm-install speedup and know their tooling tolerates the out-of-project canonical path.Verification
test/regression/issue/29614.test.ts:readlink -f node_modules/<pkg>resolves inside the project tree, andnode_modules/.bun/<storepath>is a real directory.main): the same assertion fails — the entry is a symlink into<cache>/links/.BUN_INSTALL_GLOBAL_STORE=1still opts into the shared store (the speedup is preserved for users who want it).Existing
describe("global virtual store", …)tests that exercise the shared-store layout are updated to explicitly passglobalStore: true. The "off by default" behavior itself is pinned by a new test in that same block.