install: isolated linker honors active bun link - #30289
Conversation
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds publishable-paths computation and a linked-package-name cache, uses them so isolated installs honor active Changesbun link + publishable-paths for isolated installs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/pack_command.zig`:
- Around line 958-960: The pack step is losing the optional flag for binary
entries: when adding bins to pack_queue you mark them as optional
(pack_queue.add with .{ .path = bin.path, .optional = true }) but
PublishablePaths.paths (and later pack()) returns only plain paths so missing
optional bins are treated as required and cause linker/copy failures; either
filter out non-existent optional bin entries at the point you add them (e.g.,
check file existence before calling pack_queue.add) or change the return type
from PublishablePaths.paths to include the optional metadata and propagate that
richer type through pack() and any consumers so optional bins are skipped if
missing. Update the code paths that consume pack_queue entries and
PublishablePaths.paths (including functions pack() and any code in the 1010-1015
region) to respect the optional flag.
- Around line 968-999: The code currently treats a present-but-not-array "files"
field as if it were absent and falls back to iterateProjectTree, causing
inconsistency with pack(); instead, detect the malformed manifest and abort the
command: in the branch handling json_root.get("files") where files.asArray() is
false, replace the fallback logic with a manifest error return (or call the
existing manifest validation error/reporting helper) so the function (the
pack_command path that calls iterateIncludedProjectTree / iterateProjectTree and
ultimately pack()) fails fast on non-array "files" entries rather than widening
the published file set.
- Around line 945-1021: collectPublishablePaths currently never walks vendored
bundled dependencies, so packages relying on bundledDependencies are incomplete;
modify collectPublishablePaths to mirror pack() by detecting the package
manifest's bundledDependencies (e.g. json_root.get("bundledDependencies") or the
equivalent field you expect) and invoke the existing iterateBundledDeps function
to add those files into pack_queue (similar to how bins and files are
processed). Locate collectPublishablePaths and after processing bins/files
(before final iterateProjectTree fallback), call iterateBundledDeps(allocator,
&pack_queue, root_dir, json_root, .silent) or the correct iterateBundledDeps
signature used elsewhere, ensuring bundled deps are queued into pack_queue so
they appear in the returned paths array. Ensure errors are propagated
consistently (use try) and that iterateBundledDeps runs with the same
allocator/arena so added paths live in the returned arena.
In `@src/install/isolated_install.zig`:
- Around line 1589-1598: The code currently computes has_active_link but only
forces a rebuild and doesn't prevent a linked producer from being considered for
the global virtual store; modify the eligibility logic so that when
has_active_link is true the package is treated as ineligible for the global
store before the entry_hash/eligibility pass runs (i.e., ensure
entryUsesGlobalStore or the variable checked by the entry_hash pass is set to
false for that pkg_name). Update the places where has_active_link is computed
(the block around PackageInstall.supported_method / manager.linkedPackagePath)
and the other noted blocks (the analogous checks at the later occurrences) so
that linked producers are excluded from global-store eligibility rather than
merely triggering rebuilds.
In `@src/install/PackageManager/PackageManagerDirectories.zig`:
- Around line 518-540: The no-cache fallback calls globalLinkDirPath() which can
abort via Global.exit(1); change the fallback to use the same
non-fatal/open-and-check logic as populateLinkedNamesCache() so missing or
unreadable global link state returns null instead of exiting. Specifically, in
the branch after use_cache (the second call sites of globalLinkDirPath() and
subsequent bun.path.joinAbsStringBufZ/joined handling), replace the direct
globalLinkDirPath() usage with a safe lookup that attempts to open/read the
global link dir (reusing the open/path error handling from
populateLinkedNamesCache()), and if that fails return null; otherwise proceed to
join the path and perform the same Windows
(getFileAttributes/is_directory/is_reparse_point) and non-Windows
lstat/ISDIR/ISLNK checks as before.
🪄 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: 3dabccb5-e09c-4647-8054-6e19c551cf87
📒 Files selected for processing (10)
src/cli/pack_command.zigsrc/install/PackageManager.zigsrc/install/PackageManager/PackageManagerDirectories.zigsrc/install/PackageManager/WorkspacePackageJSONCache.zigsrc/install/PackageManager/patchPackage.zigsrc/install/isolated_install.zigsrc/install/isolated_install/FileCopier.zigsrc/install/isolated_install/Hardlinker.zigsrc/install/isolated_install/Installer.zigtest/cli/install/isolated-install.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/install/isolated_install/Installer.zig`:
- Around line 763-775: The EXDEV fallback currently jumps from
linkedHardlinkPaths() to linkedCopyPaths() (in the linked_initial_method
.hardlink branch) without removing any files already created by the partial
hardlink pass, which risks truncating producer inodes; update the .err handling
for linkedHardlinkPaths (and the analogous spots) so that on err.getErrno() ==
.XDEV you first delete/cleanup any partially created destination entries (the
staging files/links pointed to by dest or whatever structure linkedHardlinkPaths
populated) before continuing to :backend .copyfile; ensure the cleanup logic is
invoked in the same branches referenced (linked_initial_method .hardlink, the
EXDEV checks around linkedHardlinkPaths) so linkedCopyPaths()/FileCopier.copy()
runs against a clean destination.
🪄 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: 0c866757-c107-4f52-9b41-c37234d1fc08
📒 Files selected for processing (3)
src/install/PackageManager/PackageManagerDirectories.zigsrc/install/isolated_install/Installer.zigtest/internal/ban-limits.json
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/install/isolated_install/Installer.zig`:
- Around line 795-809: The fallback is constructing src by starting from
initTopLevelDirLongPath() then appendJoin(producer_path), which yields an
incorrect path when producer_path is already absolute; change the src
construction to use the absolute producer path directly (i.e., initialize src
from producer_path or from linkedPackagePath() result) so Hardlinker is given
the real linked producer tree; update the code around the src variable creation
(used when calling Hardlinker.init with folder_dir, src, dest,
linked_skip_files, linked_skip_dirs) to avoid prepending the project root.
In `@test/cli/install/isolated-install.test.ts`:
- Around line 2187-2219: The test currently only asserts that the isolated
installer used the registry body (bodyDir) but doesn't verify the consumer's
node_modules points at the linked producer; add an assertion that
node_modules/no-deps exposes the producer tree by checking the presence of the
producer marker (use packageDir and join to build join(packageDir,
"node_modules", "no-deps", "marker.js") and assert existsSync(...) is true or
assert the top-level node_modules/no-deps is a symlink to the producer); update
the test after the existing bodyDir checks (symbols: packageDir, producer,
bodyDir, join, existsSync) so the test fails if --backend=symlink stopped
pointing consumers at the linked producer.
🪄 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: 20b800db-75ce-4e1a-8044-9b3cc2f6ba8e
📒 Files selected for processing (3)
src/install/PackageManager/PackageManagerDirectories.zigsrc/install/isolated_install/Installer.zigtest/cli/install/isolated-install.test.ts
8d8aa19 to
19dc6ef
Compare
|
The Rust rewrite (#30412) landed while this PR was open. All the install-side changes here are in The logic to port (from the
The test suite under the I do not have the iteration budget for this port in this session; flagging for a maintainer familiar with both sides of the rewrite. |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/install/isolated_install.rs:2277-2282— The Rust port omits theif has_active_link { installer.start_task(entry_id); continue; }short-circuit that the Zig version adds at isolated_install.zig:1709-1716.has_active_linkis OR'd intoneeds_installhere, but after the!needs_installblock closes at line 2347, control falls straight through tocache_subpath_zresolution andenqueue_package_for_download— so a linked package still hits the registry even though the worker (Installer.rs:1068+) will source the body from the producer dir and never read the downloaded bytes. This is the PR description's first design bullet ("Skip the cache-fetch dance") not ported to Rust; add the same short-circuit immediately after line 2347.Extended reasoning...
What the bug is
The Zig side of this PR adds, immediately after the
if (!needs_install) { … continue; }block inisolated_install.zig:// `link_package` will source from the producer dir via // `linkedPackagePath`; skip the cache-fetch dance entirely // (mirrors how `.folder` is handled — no registry traffic // needed when the body comes from an on-disk producer). if (has_active_link) { installer.startTask(entry_id); continue; }
This is the PR description's first fix bullet: "Skip the cache-fetch dance. The body comes from the on-disk producer, not the registry, so the download/extract task for that entry never runs."
The Rust port (
isolated_install.rs) only does half of this: it computeshas_active_link(lines 2229-2241) and ORs it intoneeds_install(line 2280) so that an existing store entry doesn't short-circuit the override. But it does not add the corresponding short-circuit between the!needs_installblock and the cache-subpath / download-enqueue logic.Code path
At isolated_install.rs:2320-2452:
- Line 2280:
needs_install = … || has_active_link || …→truefor a linked package. - Lines 2320-2347:
if !needs_install { … continue; }— skipped becauseneeds_installis true. - Line 2351 (immediately after, no intervening check):
let cache_subpath_z = match pkg_res_tag { ResolutionTag::Npm => cached_npm_package_folder_name(…), … }. - Line 2389:
missing_from_cacheis computed by checking whether<cache>/<pkg>@<ver>/package.jsonexists. - If the tarball isn't already cached (cold install / fresh machine / private package not yet published), line 2452 calls
manager.enqueue_package_for_download(…)withTaskCallbackContext::IsolatedPackageInstallContext(entry_id).
The download/extract runs to completion, and only then does the callback fire
installer.start_task(entry_id). The worker'sTask.run(Installer.rs:1068-1170) then re-checkslinked_package_path, finds the producer, and copies from it — never reading the downloaded/extracted bytes.Why existing code doesn't prevent it
There is simply no branch between lines 2347 and 2351 that consults
has_active_link. The variable is computed and used once (in theneeds_installchain) but the purpose of that boolean — to make the entry skip both the warm-hit fast path and the registry fetch — is only half-implemented. The Zig reference and the PR description both document the missing half explicitly. The robobun comment on this PR even calls out item 5 as needing porting ("has_active_linkcheck and link-override block in the per-entry installer loop"); the override block landed in Installer.rs but the main-thread scheduling half didn't fully land in isolated_install.rs.Step-by-step proof
On a machine with a fresh tarball cache (or
BUN_INSTALL_CACHE_DIRpointed at an empty dir, as the PR's own hermetic tests do):- Producer at
/tmp/prodwith{"name":"lodash","version":"4.17.21"}; user runsbun link. - Consumer (isolated linker) depends on
lodash@^4.17.0. User runsbun install. populate_linked_names_cache()puts"lodash"inlinked_names.- Main loop reaches
lodash@4.17.21:has_active_link = true(line 2238),needs_install = … || true = true. !needs_installis false → skip lines 2320-2347.- Line 2351:
cache_subpath_z = "lodash@4.17.21". Line 2389:<cache>/lodash@4.17.21/package.jsondoesn't exist →missing_from_cache = true. - Line 2452:
enqueue_package_for_download("lodash", …, 4.17.21, registry_url, IsolatedPackageInstallContext(entry_id), …). - Bun fetches
lodash-4.17.21.tgzfrom the registry, extracts it into<cache>/lodash@4.17.21/, and only then fires the callback →start_task(entry_id). - Worker enters the override block in Installer.rs, opens
/tmp/prodvia the global-link symlink, andFileCopiers the producer tree into the store entry. The freshly-extracted<cache>/lodash@4.17.21/is never opened.
In Zig, step 6 never happens:
if (has_active_link) { installer.startTask(entry_id); continue; }jumps straight to step 9.Impact
- Wasted network/disk/latency on every cold install of a linked package — exactly what the PR's first design bullet says it eliminates. The new
bun link integrationtests in this PR all usehermeticEnvwith a freshBUN_INSTALL_CACHE_DIR, so every test run downloadsno-deps@1.0.0from Verdaccio unnecessarily. - Correctness in offline / unpublished scenarios: if the registry is unreachable (offline dev, air-gapped CI) or the linked package hasn't been published yet (the canonical
bun linkuse case — developing a package before its first publish),enqueue_package_for_downloadfails andon_package_download_errorfires instead ofstart_task. The linked package never installs even though its producer is sitting on disk. The Zig version handles this correctly; the Rust version regresses it.
Fix
Insert immediately after line 2347 (mirroring isolated_install.zig:1709-1716):
// `link_package` will source from the producer dir via // `linked_package_path`; skip the cache-fetch dance entirely // (mirrors how `.folder` is handled — no registry traffic // needed when the body comes from an on-disk producer). if has_active_link { installer.start_task(entry_id); continue; }
- Line 2280:
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/install/isolated_install.rs:2282— The Rust port is missing theif has_active_link { installer.start_task(entry_id); continue; }early dispatch that the Zig reference inserts at isolated_install.zig:1713–1716 between the!needs_installblock and the cache-subpath/download logic. As a result, whenhas_active_linkis true and the tarball isn't cached, control falls through toenqueue_package_for_download; if that download fails (offline / 404 / private-registry auth), the entry is marked Done withTaskError::Downloadand the worker override never runs — the on-disk producer is never consulted. Fix: insert the dispatch between line 2349 and thecache_subpath_zmatch at line 2353, mirroring the Zig hunk.Extended reasoning...
What the bug is
The Zig reference in this same PR (isolated_install.zig diff hunk
@@ -1657,6 +1706,15 @@) inserts an early dispatch between the!needs_installcontinue and the cache-subpath / download-enqueue logic:// `link_package` will source from the producer dir via // `linkedPackagePath`; skip the cache-fetch dance entirely // (mirrors how `.folder` is handled — no registry traffic // needed when the body comes from an on-disk producer). if (has_active_link) { installer.startTask(entry_id); continue; }
The PR description lists "Skip the cache-fetch dance" as a core design point. The Rust port (
isolated_install.rs) only ORshas_active_linkintoneeds_installat line 2282 and has no equivalent early dispatch —grepconfirmshas_active_linkappears only at lines 2229 (declaration) and 2282 (the OR). After theif !needs_install { … continue; }block closes at line 2349, control falls straight into thecache_subpath_zmatch at line 2353, then themissing_from_cachecheck at line 2391, then the download-enqueue block (enqueue_package_for_download/enqueue_git_for_checkout/enqueue_tarball_for_download) when the tarball isn't cached.Per robobun's 2026-05-15 comment, the
.zigfiles are no longer compiled post-#30412 — the.rspath is what ships. The robobun port-checklist explicitly lists item 5 ("has_active_linkcheck and link-override block in the per-entry installer loop") as not-yet-ported; the port added the boolean and the OR intoneeds_install, but not the dispatch-and-continue.Why existing code doesn't prevent it
ORing
has_active_linkintoneeds_installonly ensures the entry isn't skipped — it does nothing to bypass the cache-fetch / download path that follows. The worker-side override inInstaller.rs(lines 1068+) only runs once a task is actually started; if the main thread enqueues a download instead, the download's failure callback marks the entryStep::Doneand callson_task_fail(entry_id, TaskError::Download(...))beforeTask::runis ever scheduled.Step-by-step proof
- User runs
bun linkin a producer dir for a package whose registry version is unavailable (private registry with expired auth, package not yet published, or the user is offline). - User runs
bun install(isolated linker) in a consumer that depends on that package name. populate_linked_names_cache()adds the name tolinked_names. Main loop reaches the entry:has_active_link = true(line 2229), soneeds_install = … || true = true(line 2282).- Line 2322:
if !needs_install { … }is skipped. No early dispatch follows. Line 2353 computescache_subpath_z; line 2391 evaluatesmissing_from_cache— the tarball was never downloaded (first install / cold cache), sotrue. - Control reaches
manager.enqueue_package_for_download(..., TaskCallbackContext::IsolatedPackageInstallContext(entry_id)). The HTTP request fails (401 / 404 / ENETUNREACH). - The download-failure callback marks
entry_steps[entry_id] = Step::Doneand callson_task_fail(entry_id, TaskError::Download(...)). The install reports the package as failed. - The worker's
Task::runoverride block (Installer.rs:1068+) never runs, so the producer sitting on disk at<globalLinkDir>/<pkg>is never consulted.
This defeats a primary use-case of
bun link: developing against a local producer when the registry version is unavailable or stale. Even when the download succeeds, every reinstall of a linked package whose tarball isn't cached pays a wasted registry round-trip + tarball extract that the worker then ignores.Why tests don't catch it
All
bun link integrationtests depend onno-deps@1.0.0, which is published to the verdaccio test registry, so the download always succeeds and the worker override runs afterward. No test exercises offline / registry-down / 404 with an active link.Relationship to other comments
This is distinct from inline comments #3247469973 (
assume_mutUB) and #3247469977 (worker-side missing detachment / skip-lists / eligibility-DFS) — those target the worker-side override block inInstaller.rs. This is the main-thread scheduling gap inisolated_install.rs: the worker override can't run if the main thread routes the entry through a failing download instead ofstart_task.Fix
Insert between line 2349 (end of
if !needs_install { … }) and line 2353 (let cache_subpath_z = match pkg_res_tag { … }):// `link_package` will source from the producer dir via // `linked_package_path`; skip the cache-fetch dance entirely // (mirrors how `.folder` is handled — no registry traffic // needed when the body comes from an on-disk producer). if has_active_link { installer.start_task(entry_id); continue; }
matching isolated_install.zig:1713–1716.
- User runs
|
Re: review 4297435226 — this was already fixed in 3b88b148 ("install: short-circuit linked packages past the cache-fetch dance"), one commit after the Re: the test-coverage gap — the reviewer is correct that no existing test exercises offline/404-with-active-link. I tried adding one pointed at |
cae9bcc to
76cdadc
Compare
de419c4 to
79926c7
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
95008cc to
8ba97f3
Compare
|
Re the The batch did surface one genuine defect: when I added the |
8ba97f3 to
c346021
Compare
| fn resolves_inside(fd: Fd, root: &[u8]) -> bool { | ||
| let mut buf = PathBuffer::uninit(); | ||
| match sys::get_fd_path(fd, &mut buf) { | ||
| Ok(p) => p.starts_with(root) && (p.len() == root.len() || p[root.len()] == SEP), | ||
| Err(_) => true, | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 resolves_inside (added in ae5abac for the resolved 🔴 at 10:35:06Z) only rejects isolated-linker global installs when globalStore is off. With install.globalStore = true / BUN_INSTALL_GLOBAL_STORE=1, <globalDir>/node_modules/.bun/<storepath> is an absolute symlink into <cache>/links/<hash>/ (Installer.rs:2833), so openat(O_DIRECTORY) on <globalDir>/node_modules/<pkg> resolves into ~/.bun/install/cache/links/… — outside <globalLinkDir> (~/.bun/install/global/node_modules) — and is_linked_entry returns true. Same gap in linked_package_path_mut and the Windows arm of linked_package_path. Every consumer then FileCopiers the globally-installed version into .bun/<pkg>@<locked-ver>/ — the exact silent-substitution class the check was added for. Fix: readlinkat the immediate target and reject relative .bun/… targets (a real bun link registration is always a single-hop absolute symlink to the producer), or additionally reject targets under <cache>/links/. The new "global install with isolated linker is not treated as bun link" test doesn't set globalStore, so add a variant that does.
Extended reasoning...
What the bug is
resolves_inside (PackageManagerDirectories.rs:880-886) was added in commit ae5abac to close the resolved 🔴 review comment at 2026-08-14T10:35:06Z: bun add -g <pkg> --linker=isolated drops <globalDir>/node_modules/<pkg> as a symlink, and without a target check that symlink was misclassified as a bun link registration. The fix follows the symlink via openat(O_DIRECTORY), calls get_fd_path on the resulting fd, and rejects when the resolved path starts_with(<globalLinkDir>). The doc comment and PR description state the assumption: "isolated-linker global installs land as symlinks into the global dir's own .bun store, so candidates are additionally rejected when their resolved target lies back inside the link dir".
That assumption only holds when install.globalStore (GVS) is off. When it's on, link_project_to_global_store (Installer.rs:2826-2848) makes <globalDir>/node_modules/.bun/<storepath> an absolute symlink into <cache_dir>/links/<storepath>-<hash>/ — the comment at line 2833 says exactly "Absolute target so the link is independent of where node_modules lives". And <cache_dir> (fetch_cache_directory_path → ~/.bun/install/cache/) is a sibling of <globalDir> (~/.bun/install/global/), not a child of <globalLinkDir>.
Why the config is reachable
The GVS gate at isolated_install.rs:1226 reads only manager.options.enable.global_virtual_store(), with no !options.global carve-out — so bun add -g <pkg> --linker=isolated with GVS enabled uses the global virtual store for the global install too. And the population that sets linker = "isolated" in a user-level bunfig is precisely the population likely to also set globalStore = true there (both are opt-in isolated-linker features documented together).
Step-by-step proof
Setup: user-level ~/.bunfig.toml has install.linker = "isolated" and install.globalStore = true. User runs bun add -g typescript (resolves to, say, 5.6.2).
On-disk state after the global install:
~/.bun/install/global/node_modules/typescript→.bun/typescript@5.6.2/node_modules/typescript(relative symlink — the isolated linker's standard top-level shape)~/.bun/install/global/node_modules/.bun/typescript@5.6.2→~/.bun/install/cache/links/typescript@5.6.2-<hash>/(absolute symlink, perlink_project_to_global_store)
Now in any consumer project with "typescript": "5.4.5" as a direct dep, install_isolated_packages calls populate_linked_names_cache:
- readdir of
<globalLinkDir>=~/.bun/install/global/node_modules/yields entrytypescriptwithkind == SymLink. is_linked_entry(SymLink, root_fd, "typescript", <globalLinkDir>):is_symlink = true→openat(root_fd, "typescript", O_DIRECTORY | O_RDONLY)follows the full chain (typescript→.bun/typescript@5.6.2/node_modules/typescript→ through the absolute GVS symlink →~/.bun/install/cache/links/typescript@5.6.2-<hash>/node_modules/typescript) and opens that directory.resolves_inside(fd, "~/.bun/install/global/node_modules"):get_fd_path(fd)returns~/.bun/install/cache/links/typescript@5.6.2-<hash>/node_modules/typescript. That does not.starts_with("~/.bun/install/global/node_modules")→ returns false.is_linked_entryreturns!false= true →"typescript"is inserted intolinked_names.- The
linked_pkg_idsbuild callslinked_package_path_mut(manager, "typescript", …): the POSIX fast-pathlinked_names.contains_key("typescript")passes;lstaton<globalLinkDir>/typescriptsees a symlink → passes;open_dir_for_iteration+resolves_insidefails the same way (resolves into<cache>/links/…, not inside<globalLinkDir>) → returnsSome(joined).linked_pkg_ids.set(typescript_pkg_id). - Main-thread scan:
has_active_link = true→ skips the registry fetch,start_taskimmediately. - Worker override block:
linked_package_pathreturns<globalLinkDir>/typescript;open_dir_for_iterationon it opens<cache>/links/<hash>/node_modules/typescript(typescript 5.6.2); FileCopier copies that into the consumer'snode_modules/.bun/typescript@5.4.5/node_modules/typescript/and drops a.bun-linkmarker.
The consumer's lockfile says 5.4.5; the bytes on disk are 5.6.2. No error, no warning. Same failure in the Windows arm of linked_package_path (line ~1160: same open_dir_for_iteration + resolves_inside(fd, dir_path_ref) shape).
Why existing code doesn't prevent it
resolves_inside checks only one prefix (<globalLinkDir>), and the GVS symlink escapes exactly that prefix by design ("Absolute target so the link is independent of where node_modules lives"). The new test "isolated: global install with isolated linker is not treated as bun link" writes install: { linker: "isolated", registry: … } to the user bunfig but does not set globalStore, so its global add lands entirely inside <globalLinkDir>/.bun/… and resolves_inside correctly rejects it — the test never exercises the GVS path.
Impact
Exactly the class the resolved 🔴 comment described and resolves_inside was added to prevent: after bun add -g <pkg> --linker=isolated with GVS on, every isolated-linker project on the machine listing <pkg> as a direct dep silently gets the globally-installed version copied into its .bun/<pkg>@<locked-ver>/ store, plus a .bun-link marker that forces a rebuild every install. Per REVIEW.md "Fix the whole class in the same PR" and "Cover the variant matrix, not just the repro", the GVS-on variant is a same-class site that belongs here.
How to fix
The cleanest discriminator is the immediate symlink target, not the fully-resolved fd path: readlinkat(dir_fd, name, buf) and reject when the target is relative (or specifically starts with .bun/). A real bun link registration is always a single-hop absolute symlink to the producer's working tree (that's what bun link writes); an isolated-linker global install's top-level entry is always a relative .bun/<storepath>/node_modules/<pkg> symlink, regardless of GVS. This closes both the GVS-off case (which resolves_inside already handles) and the GVS-on case with one check, and avoids having to compute <cache_dir>/links (which would need fetch_cache_directory_path + the env loader) at every is_linked_entry call site. Alternatively, keep resolves_inside and additionally reject when the resolved path lies under <cache_dir>/links/. Either way, add a globalStore: true variant of the existing "global install with isolated linker" test.
| let linked_pkg_ids: DynamicBitSet = { | ||
| let mut set = DynamicBitSet::init_empty(lockfile.packages.len())?; | ||
| let any_links = if cfg!(windows) { | ||
| manager.linked_names_any_on_windows | ||
| } else { | ||
| !manager.linked_names.is_empty() | ||
| }; | ||
| if any_links | ||
| && PackageInstall::supported_method() != crate::package_install::Method::Symlink | ||
| { |
There was a problem hiding this comment.
🔴 The link override fires during global installs: install_isolated_packages is called for bun add -g --linker=isolated (install_with_manager.rs:833 dispatches on node_linker alone), and the linked_pkg_ids build at lines 252-301 has no !manager.options.global gate. If <pkg> is both bun link-registered and a direct dep of the global root, has_active_link skips the registry fetch and the worker FileCopiers the producer's WIP tree into <globalDir>/node_modules/.bun/<pkg>@<ver>/ — the same silent-substitution class resolves_inside was added (ae5abac) to prevent in the consumer→global direction, in reverse. Gate any_links on && !manager.options.global; the link override is a consumer-project feature, and a global install IS the link dir.
Extended reasoning...
What the bug is
The linked_pkg_ids bitset build at isolated_install.rs:252-301 runs unconditionally for every isolated install with no check of manager.options.global, and install_isolated_packages is reached during global installs — install_with_manager.rs:802-840 dispatches on manager.options.node_linker alone (no global override to hoisted). A grep of isolated_install.rs for options.global returns zero matches. So during bun add -g <pkg> --linker=isolated (or any global install on a machine with a user-level linker = "isolated" bunfig), the global root's direct dependencies are scanned against the bun link registry, and any package that is both link-registered AND a direct dep of the global package.json gets the producer-override applied — inside the global install itself.
The specific code path
- User
bun links their WIP fork oflodash→<globalDir>/node_modules/lodashis a symlink to~/dev/lodash(outside the link dir). - User runs
bun add -g lodash --linker=isolated.install_with_manager.rs:727callssetup_global_dir; line 833 dispatches toinstall_isolated_packages(nooptions.globalcheck in the linker match). populate_linked_names_cache(isolated_install.rs:232) readdirs<globalDir>/node_modules/, finds thelodashsymlink.is_linked_entryseesSymLink, follows it,resolves_inside(fd, <globalDir>/node_modules)returns false (target is~/dev/lodash, outside), sois_linked_entryreturns true and"lodash"is added tolinked_names.linked_pkg_idsbuild (lines 252-301):any_linksis true (line 254-258, no global gate),bun addjust madelodasha direct dep of the global root,linked_package_path_mut("lodash")at line 288 sees the symlink →Some, soset.set(res)fires.- Main-thread scan:
has_active_link = true(line 2291) is OR'd intoneeds_install(line 2299) and short-circuits the cache-fetch dance at lines 2437-2440 — the registry is never consulted. - Worker override in
Installer.rs:1098-1356opens~/dev/lodash,delete_trees the global entry, andFileCopiers the producer's working tree into<globalDir>/node_modules/.bun/lodash@<ver>/node_modules/lodash/, dropping a.bun-linkmarker.
The user asked for a global registry install and silently got their WIP source tree instead.
Why existing code doesn't prevent it
The PR added two guards against confusing global-dir contents with link registrations, and neither covers this direction:
resolves_inside(commit ae5abac, PackageManagerDirectories.rs:880) rejects candidates whose target resolves inside the link dir — designed so an isolated global install's own.bunsymlinks aren't mistaken for links by consumer projects. But a realbun linkregistration points outside the link dir by construction, so it correctly passesresolves_insideand is admitted. The guard protects consumers from globals; it doesn't protect the global install from itself.- The symlink-only filter in
is_linked_entryrejects hoisted global installs (real directories). Irrelevant here — the entry IS a realbun linksymlink.
The only remaining discriminator is "is this a global install?" — and isolated_install.rs never checks it.
Impact
Silent version substitution in a global install: the user runs bun add -g lodash expecting the published registry bytes and gets their local WIP tree, with no warning. This is the exact bug class the PR description names ("silent footgun") and that resolves_inside was added to prevent in the consumer→global direction — here it fires in reverse. It also breaks the PR's stated design goal of hoisted-linker parity: hoisted bun add -g clobbers the link symlink with a fresh registry directory and does not source from the producer.
The scenario is niche (requires both an active bun link on <pkg> AND a bun add -g <pkg> with the isolated linker), but it's realistic — a developer linking their fork of a CLI tool and then trying to also install it globally is a plausible workflow — and the failure is silent. The "poisons every subsequent global install" tail is likely overstated (after the first install, the top-level <globalDir>/node_modules/lodash becomes an isolated-store symlink that resolves_inside rejects on the next scan, and the .bun-link marker triggers a rebuild), but the first install is definitively wrong.
Step-by-step proof
Take POSIX, $BUN_INSTALL=~/.bun, isolated linker via user bunfig:
cd ~/dev/lodash && bun link→~/.bun/install/global/node_modules/lodash→~/dev/lodash(symlink).bun add -g lodash@4.17.21→options.global = true,options.node_linker = Isolated, globalpackage.jsongains"lodash": "4.17.21"as a direct dep.install_with_manager.rs:802readsIsolated, line 833 callsinstall_isolated_packages(manager, ...).- Line 232:
populate_linked_names_cache(manager).global_link_dir_pathresolves to~/.bun/install/global/node_modules. Readdir yieldslodashwithkind == SymLink.is_linked_entry(SymLink, root_fd, "lodash", link_dir_root):is_symlink = true;openat(root_fd, "lodash", O_DIRECTORY)follows to~/dev/lodash, succeeds;resolves_inside(fd, "~/.bun/install/global/node_modules")→get_fd_path(fd)=~/dev/lodash, does NOT start with root → false →is_linked_entryreturns true.linked_names.put("lodash", ()). - Line 254:
any_links = !linked_names.is_empty()= true. Line 259:any_links && backend != Symlink= true — no!options.globalterm. Line 270:scan_targets = [0](global root). Line 281-297: for the global root's direct deplodash(res = pkg_id of lodash@4.17.21),linked_package_path_mut(manager, "lodash", ...):linked_names.contains_key("lodash")= true;lstat(<linkdir>/lodash)→ symlink →is_link = true;open_dir_for_iterationsucceeds,resolves_inside= false → returnsSome.set.set(res). - Line 2291:
has_active_link = linked_pkg_ids.is_set(pkg_id)= true. Line 2299:needs_install = ... || has_active_link || ...= true. Line 2437:if has_active_link { installer.start_task(entry_id); continue; }— registry fetch skipped. - Worker
Task::run,Installer.rs:1098:linked_pkg_ids.is_set(pkg_id) && backend != Symlink= true.linked_package_path(manager, "lodash", ...)=Some("~/.bun/install/global/node_modules/lodash").open_dir_for_iterationfollows the symlink to~/dev/lodash.delete_tree(<globalDir>/node_modules/.bun/lodash@4.17.21/...);FileCopier(~/dev/lodash → <globalDir>/node_modules/.bun/lodash@4.17.21/node_modules/lodash); drop.bun-link.
Global lodash now contains ~/dev/lodash's WIP files, not the registry tarball.
How to fix
One-line gate at line 259:
if any_links
&& !manager.options.global
&& PackageInstall::supported_method() != crate::package_install::Method::Symlink
{The link override is a consumer-project feature ("my project should see my WIP fork instead of the published version"). A global install is not a consumer of the link — it IS the link dir. The PR's own test "global install with isolated linker is not treated as bun link" covers the consumer→global direction; this needs a sibling test for the global-install→producer direction (bun link a producer, then bun add -g the same name with --linker=isolated, assert the global .bun entry contains registry bytes and no .bun-link marker).
| /// `resolves_inside` checks the fully-resolved path, which is not enough | ||
| /// with `globalStore` enabled: the global install's store entry is then an | ||
| /// absolute symlink into `<cache>/links/`, so the top-level entry's chain | ||
| /// resolves outside the link dir even though it is a store entry. The | ||
| /// *immediate* target discriminates in every mode: `bun link` writes a | ||
| /// single absolute symlink to the producer tree (link_command.rs), while | ||
| /// the isolated linker's top-level entries are relative `.bun/...` | ||
| /// symlinks on POSIX (`Symlinker.target` is `RelPath`) and | ||
| /// absolute-inside-the-link-dir junctions on Windows. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // A global install IS the link dir: `bun add -g` of a | ||
| // link-registered name must install registry bytes (hoisted | ||
| // parity: the registration gets clobbered, not sourced from | ||
| // the producer's working tree). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
I reviewed this PR again after the latest two commits (scoped-link test coverage in f6ea34b and immediate-target discrimination in 570cc5f) and the bug-hunting pass found nothing new. All prior findings — the bun unlink recovery marker, the read-only global-dir probe, the isolated-linker bun add -g misclassification, the marker-check path reuse, and scoped-name coverage — are addressed and resolved.
Given the scope (new link-override machinery in the isolated installer, worker-thread &/&mut split on PackageManager, GVS-eligibility carve-outs, per-platform symlink discrimination), a maintainer sign-off is still worthwhile before merge.
What was reviewed: the linked_pkg_ids bitset build and its read-only use from workers against the Task::run SAFETY contract; link_target_is_outside / resolves_inside against bun add -g --linker=isolated with and without globalStore; the .bun-link marker write/detect path against the PatchInfo::Remove analogue; the open_global_dir → global_dir_path refactor for behavior preservation. A Windows case-sensitivity concern in the byte-prefix containment checks was examined and ruled out — global_link_dir_path is set from get_fd_path on the same handle both sides compare against, so casing is consistent.
Extended reasoning...
Overview
This PR makes the isolated linker honor active bun link registrations, closing #30287. It adds ~490 lines to PackageManagerDirectories.rs (link-dir scan, is_linked_entry / resolves_inside / link_target_is_outside discrimination, read-only linked_package_path and main-thread linked_package_path_mut), ~120 lines to isolated_install.rs (the linked_pkg_ids bitset scoped to root/workspace direct-dep resolutions, GVS-eligibility carve-out, .bun-link marker recovery, fetch-skip), ~280 lines to Installer.rs (worker-side override: stale-GVS-symlink detachment, delete_tree + FileCopier from the producer, marker write), a small FileCopier::init_with_skip extension, three new PackageManager fields, and a global_dir_path / open_global_dir refactor in PackageManagerOptions.rs. Tests add a ~965-line describe("bun link integration") block with 12 active tests + 3 test.todos.
Security risks
None identified. The link override reads from a directory the user's own bun link populated under $BUN_INSTALL; there is no untrusted-archive path handling here. The resolves_inside / link_target_is_outside checks fail closed (unresolvable → treated as inside → registry fallback). The delete_tree in the worker override is debug_assert!-guarded to be project-local via the GVS carve-out, and the stale-GVS-symlink unlink now propagates non-ENOENT errors instead of writing through into the shared cache.
Level of scrutiny
High. This is a ~700-line native feature in the package-manager install path — production-critical, runs on every isolated bun install, and threads state through the main-thread → worker boundary under an explicit SAFETY contract. It introduces design decisions a maintainer should ratify: overriding by direct-dep resolution (not name), forcing link-overridden entries GVS-ineligible, copyfile-only materialization, the .bun-link marker convention, and default-excludes-only file selection (pack parity deferred). The Windows path is a distinct code branch with a per-call attribute probe and the scope-flattening follow-up is explicitly deferred.
Other factors
The PR has iterated extensively (55 iterations per the evidence marker); every finding I raised across five prior review passes today has a corresponding fix commit and is marked resolved. Test coverage is thorough for the POSIX path (npm/catalog resolution, rebuild propagation, unlink recovery, negative control, --backend=symlink bypass, dangling link, multi-version scoping, scoped names, and both isolated-global-install rejection variants). The one candidate the verifier examined this run — case-sensitive byte-prefix comparison in resolves_inside / link_target_is_outside on Windows — was ruled out because both sides derive from get_fd_path on the same canonicalized global_link_dir_path. Not approving because the size and the design surface here warrant a human maintainer's sign-off; nothing outstanding blocks that.
570cc5f to
af05246
Compare
There was a problem hiding this comment.
I reviewed this iteration and the automated pass came up clean — every prior finding (unlink-recovery marker, read-only global-dir probe, isolated-linker bun add -g discrimination via immediate readlink target, marker-check path reuse, scoped-name coverage) is addressed in the current diff. Given the scope — ~500 lines of new worker-thread install logic with cross-platform #[cfg] branches, shared <cache>/links/ invariants a regression could silently corrupt, and a Windows path that defers to CI — a human look is still warranted before merge.
What was reviewed:
linked_pkg_idsbitset scoping (direct-dep resolutions only) and the GVS-ineligibility carve-out that keepsdelete_treeproject-local.- Stale-GVS-symlink detachment in the worker override — non-ENOENT unlink failures now fail the task instead of writing through into
<cache>/links/. link_target_is_outside+resolves_insidereject both plain andglobalStore-mode isolated global installs;!options.globalgate keepsbun add -gon registry bytes..bun-linkmarker write/read and its fold into the existingscope_for_patch_tag_pathpath reuse.
Extended reasoning...
Overview
Makes the isolated linker honor active bun link registrations for npm/catalog-resolved direct deps. Touches PackageManager.rs (three new fields), PackageManagerDirectories.rs (~480 new lines: populate_linked_names_cache, linked_package_path{,_mut}, is_linked_entry, resolves_inside, link_target_is_outside), PackageManagerOptions.rs (open_global_dir split into a pure global_dir_path + the mkdir wrapper), isolated_install.rs (bitset build, GVS carve-out, needs_install chain, fetch-skip), Installer.rs (~280-line worker override block plus LINK_OVERRIDE_MARKER), FileCopier.rs (init_with_skip), and ~965 lines of tests.
Security risks
No auth/crypto surface. The real risk is filesystem integrity of the machine-wide global virtual store: an incorrectly-scoped delete_tree or a FileCopier write through a leftover <cache>/links/ symlink would corrupt entries every project on the machine shares. The PR guards this three ways (bitset forces entry_hash = 0 so append_real_store_path is project-local, a debug_assert!(!entry_uses_global_store) before the delete, and lstat/unlink of a stale GVS symlink with hard-fail on non-ENOENT), and the discrimination logic rejects global installs by immediate readlink target. I did not find a hole, but the consequence of one is severe enough that a maintainer should sign off on the invariant.
Level of scrutiny
High. This is production-critical install-path code, not sandbox: multi-threaded workers with a documented Task::run SAFETY contract, one unsafe raw-slice reconstruction in the worker, materially divergent POSIX vs Windows branches (the Windows path re-probes per call and per the PR footer was not locally verified), and design decisions (direct-dep-only override, copyfile-only, .bun-link marker convention) that shape user-visible behavior.
Other factors
The PR is on iteration ~55 with 200+ timeline events; earlier automated rounds surfaced several 🔴 issues (GVS carve-out never ported, unlink asymmetry, global-install misclassification, mkdir side-effect in the probe) — all now resolved in the diff and covered by new tests, including the scoped-name and globalStore-mode negative cases added after the last round. Test coverage is thorough (12 active + 3 test.todo for the deferred pack-parity follow-up). The open_global_dir refactor is behavior-preserving. Nothing outstanding blocks; the deferral is purely about scope and the shared-cache blast radius, not a specific concern.
af05246 to
4101de8
Compare
There was a problem hiding this comment.
This round ran clean — no new findings, and the scoped-producer test addresses the last coverage nit. Given the size (~500 lines of new installer Rust across POSIX/Windows #[cfg] branches, worker-thread unsafe pointer reconstruction, and the GVS shared-cache carve-outs), a human pass is still worth it before merge.
What was reviewed:
- Link discrimination in
is_linked_entry/link_target_is_outside/resolves_inside— immediate-target check correctly rejects isolated-linker global installs in bothglobalStoremodes. linked_pkg_idsbitset build — the finder's&mut managerreborrow-while-holding-column-slices concern was ruled out (the raw*mut Lockfileis taken before the column borrows)..bun-linkmarker path — folded into the existing'needs_installblock reusingscope_for_patch_tag_path, so no extra path rebuild on the warm-install fast path.open_global_dirrefactor —global_dir_pathpreserves the exact env-var precedence andopen_global_dirstillmake_open_paths; the new read-only probe path leaves the tree uncreated on machines with no links (asserted by the negative-control test).
Extended reasoning...
Overview
This PR makes the isolated linker honor active bun link registrations, closing a silent-footgun gap where linked producers were ignored under linker="isolated". It touches five installer source files (~500 lines of new Rust) plus ~965 lines of tests: new PackageManager fields and a link-name cache populated once on the main thread; linked_package_path/_mut lookup helpers with distinct POSIX (hashmap) and Windows (per-call GetFileAttributesW) paths; a linked_pkg_ids bitset scoping the override to root/workspace direct-dep resolutions; a GVS-eligibility carve-out so producer bodies never land in the shared <cache>/links/ store; a ~280-line worker override block in Installer.rs that detaches stale GVS symlinks, wipes the project-local entry, FileCopiers the producer tree with default excludes, and drops a .bun-link recovery marker; and a pure refactor of open_global_dir into global_dir_path + open_global_dir so the populate step can probe read-only.
Security risks
Low but non-zero. The link-detection path reads symlink targets from a bun-owned directory and only ever narrows the set of entries treated as links (fail-closed: unresolvable → not a link). FileCopier writes are confined to project-local .bun/<storepath>/ (guarded by debug_assert!(!entry_uses_global_store) and the eligibility carve-out); the stale-GVS-symlink detachment propagates non-ENOENT unlink failures rather than writing through into the shared cache. The delete_tree of append_real_store_path(.., Which::Final) is the sharpest edge — it is safe only because the carve-out forces entry_hash = 0 for every id in linked_pkg_ids, and that coupling is enforced only by a debug assertion.
Level of scrutiny
High. This is production installer code with: cross-platform #[cfg]-gated branches (Windows paths are covered only by CI, not the local test evidence); an unsafe { from_raw_parts } reconstruction of a pooled-buffer slice on worker threads whose SAFETY comment depends on path_buffer_pool::get()'s guard semantics; a thread-safety contract (linked_names populated before workers, read-only after) that is documented but not type-enforced; and a shared-cache invariant (link-overridden ⇒ GVS-ineligible) whose violation would silently corrupt the machine-wide store. None of that is exotic for this subsystem, but it is well past the "mechanical/obvious" bar for auto-approval.
Other factors
The PR has been through ~59 iterations and every prior inline finding (including the un-bun unlink recovery gap, the isolated-linker global-install misclassification, the read-only probe, the warm-install syscall regression, and the scoped-name coverage gap) is marked resolved and reflected in the current diff. The bug-hunting system found nothing this run; the one candidate raised (the linked_pkg_ids build reborrowing &mut manager while holding lockfile column slices) was verified refuted. Test coverage is thorough (12 active + 3 todo covering npm/catalog/scoped/unlink/dangling/multi-version/global-install-rejection/globalStore/symlink-backend). No human maintainer has reviewed yet, and the design choice to copy (not symlink) the producer tree with a default-excludes list rather than pack-parity is a deliberate scope call the description flags as a follow-up — worth a maintainer nod.
1 participant
Closes #30287. Supersedes #29615 (same approach, originally in Zig; re-ported to the Rust installer after #30412 landed mid-PR).
Problem
Under the isolated linker,
bun linkin a producer andbun link <pkg>in a consumer had no effect when the consumer's dep resolves via npm or a catalog. The consumer'snode_modules/.bun/<pkg>@<ver>/node_modules/<pkg>was materialized from the registry tarball cache; the producer's working tree was never read, so edits in the producer never reached the consumer. The hoisted linker honored the link. Silent footgun.Repro
Fix (Rust installer:
isolated_install.rs,Installer.rs,PackageManagerDirectories.rs)One readdir of the global link dir at install start (
populate_linked_names_cache, main thread, before workers) seeds a name set on thePackageManager. The probe is read-only: the link dir path is computed without touching the filesystem (global_dir_path, same precedenceopen_global_diruses) and a missing dir just leaves the cache empty, so installs on machines that never ranbun linkcreate nothing under the global dir. The link dir is shared withbun add -g: hoisted global installs land as real directories (skipped by the symlink filter), and isolated-linker global installs land as symlinks into the global dir's own.bunstore, so candidates are discriminated by their immediate readlink target:bun linkwrites a single absolute symlink to the producer tree, while isolated top-level entries are relative.bun/...symlinks (POSIX) or junctions into the link dir (Windows), in everyglobalStoremode (withglobalStoreon, the chain resolves into<cache>/links/, so a resolved-path check alone is insufficient). Without that rejection abun add -g <pkg> --linker=isolatedwould silently substitute the global version into every consumer project listing<pkg>as a direct dep. The override is also gated on!options.global: a global install is the link dir itself, sobun add -gof a link-registered name installs registry bytes (hoisted parity); every subsequentlinked_package_path()is a hashmap check with no syscalls on POSIX (zero cost when no packages are linked — the CI / most-dev-machines case). Windows keeps alinked_names_any_on_windowsfast-path flag and re-probes per hit viaGetFileAttributesW+ a dangling-junction check.The override is scoped by resolution, not name: a
linked_pkg_idsbitset (built once on the main thread, read-only for workers) marks the package ids that root/workspace direct dependencies resolve to and whose name is link-registered. If the lockfile also contains the linked name at another version transitively (directno-deps@1.0.0plusno-deps@1.0.1viaone-dep), that copy keeps its registry bytes — matching the hoisted linker, which only replaces the top-levelnode_modules/<name>. (Version-matching the producer'spackage.jsonagainst the locked resolution was rejected: a WIP producer's version rarely equals the locked one, so it would silently disable the override — the original bug's failure mode.) For a marked entry, when the user didn't pick--backend=symlink:has_active_linkis OR'd intoneeds_installand short-circuits tostart_taskbefore the cache-subpath/download enqueue, so the registry is never consulted for linked entries (the canonical workflow: the producer isn't published yet).node_modules/.bun/<storepath>symlink into<cache>/links/(propagating non-ENOENT failures so a blocked unlink can't route writes into the shared cache),delete_trees the project-local entry so producer-deleted files don't persist, andFileCopiers the producer tree with default excludes (.git/.hg/.svn/CVS/node_modulesdirs;.DS_Store, lockfiles,.npmrc,bunfig.toml,.env.production, etc.).entry_hash = 0forlinked_pkg_idsmembers (the bitset is empty under--backend=symlink, matching the override guards), so mutable producer content can never land in, or be deleted from, the shared content-addressed<cache>/links/store..bun/<storepath>/...would propagate back into the producer through a shared hardlink inode.bun unlinkrecovery.bun unlinkhas no uninstall step, so the override drops a.bun-linkmarker inside the materialized body (same convention as the.bun-tagfiles).needs_installtreats marker-present-without-active-link as a rebuild, covering entries that stay project-local after unlink (trusted dependency, orglobalStoreoff, which is the default); entries that regain global-store eligibility rebuild via the global existence miss anyway. Mirrors thePatchInfo::Removeun-patch recovery.Thread-safety: workers use the read-only
linked_package_path(&PackageManager; cache +global_link_dir_pathare immutable post-populate per theTask::runSAFETY contract); the main thread useslinked_package_path_mut(lazy global-dir init) viainstaller.manager_mut()per the loop's BACKREF convention.Verification
test/cli/install/isolated-install.test.ts,describe("bun link integration")— all fail without the fix:bun unlinkthen reinstall restores the registry body (and removes the marker)--backend=symlinkbypasses the overridebun unlink) falls back to the registryno-deps@1.0.1copy (viaone-dep) keeps its registry bytesThree pack-parity tests (
package.json#fileswhitelist ===bun pm pack --dry-run) aretest.todo, see follow-ups.Scope / follow-ups
collectPublishablePaths(shared withbun pm pack); the Rust port ships default-excludes only. Porting that helper (plusbundledDependencies/ optional-binsemantics) unlocks the threetest.todos.@scope/nameflattening inpopulate_linked_names_cache(currently served by the per-call attribute probe;Name::slice_u8()already exposes the UTF-8 transcode, so the WTF-16 limitation noted in earlier reviews no longer applies).Credit to @Kniggishood for the original approach in #29615.
Rebase note
Rebased onto current main (single commit; net diff is the 5
src/install/*.rsfiles plus the test file). Latest rebase resolved one trivial conflict: #39157'sTimingsenum binding landed where thelinked_pkg_idsbitset build sits inisolated_install.rs; kept both, bitset directly before thebuild_store()call. The prior rebase resolved #38333's extraction of the inline store build intobuild_store(); every other hunk (eligibility carve-out, installer field,needs_installterms, fetch skip) merged cleanly into the refactored layout.Earlier rebase resolutions
needs_installearly-start comment inisolated_install.rs(kept thehas_active_linkskip-the-fetch block, re-applied main's adjacent comment); thePackageManagerstruct field list (kept thelinked_namesfields alongside main's now-pub(crate)on_wake, dropping the removedci_mode); theisolated-install.test.tsimport block (unioned both sides) and the test body (kept main's newhoistdescribe next to thebun link integrationblock).Fd::from_std_dirwas removed (nowDir::fd());DirEntry::name::as_zstr()became private (added a smallname_zstrhelper that copies the dirent name into a NUL-terminated buffer);FileCopier::init_with_skiptripped the newly-enabled-D unreachable-pub(nowpub(crate)).Full suite green locally (78 pass, 3 todo, 0 fail).
no test proof · iteration 63 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/isolated-install.test.ts