From 1f6d2fdc8a90dc0d28eed35d01f0e836d3a38255 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 18 Sep 2026 01:27:37 +0000 Subject: [PATCH 01/10] install: do not fail the isolated install when an optional dependency's tarball cannot be downloaded --- src/install/PackageManager/runTasks.rs | 104 ++++++++++++++------- src/install/isolated_install.rs | 30 ++---- src/install/isolated_install/Installer.rs | 83 +++++++++++----- src/jsc/AsyncModule.rs | 1 + test/cli/install/bun-install-retry.test.ts | 78 +++++++++++++++- 5 files changed, 210 insertions(+), 86 deletions(-) diff --git a/src/install/PackageManager/runTasks.rs b/src/install/PackageManager/runTasks.rs index 3feca04e87af..67c3b5fd5a3f 100644 --- a/src/install/PackageManager/runTasks.rs +++ b/src/install/PackageManager/runTasks.rs @@ -80,6 +80,9 @@ pub trait RunTasksCallbacks { // `package_id: PackageID` otherwise. Static dispatch via two trait // methods lets impls receive the correctly-typed id without a // `Task::Id` round-trip pun. + // + // `is_required` is false when only optional dependencies need the + // download. `run_tasks` has then already logged the failure as a warning. fn on_package_download_error_store( _ctx: &mut Self::Ctx, _task_id: Task::Id, @@ -87,6 +90,7 @@ pub trait RunTasksCallbacks { _resolution: &bun_install::Resolution, _err: crate::Error, _url: &[u8], + _is_required: bool, ) { unreachable!() } @@ -97,6 +101,7 @@ pub trait RunTasksCallbacks { _resolution: &bun_install::Resolution, _err: crate::Error, _url: &[u8], + _is_required: bool, ) { unreachable!() } @@ -133,9 +138,9 @@ struct ErasedCallbacks { is_store_installer: bool, on_package_manifest_error: fn(*mut (), &[u8], crate::Error, &[u8]), on_package_download_error_store: - fn(*mut (), Task::Id, &[u8], &bun_install::Resolution, crate::Error, &[u8]), + fn(*mut (), Task::Id, &[u8], &bun_install::Resolution, crate::Error, &[u8], bool), on_package_download_error_pkg: - fn(*mut (), PackageID, &[u8], &bun_install::Resolution, crate::Error, &[u8]), + fn(*mut (), PackageID, &[u8], &bun_install::Resolution, crate::Error, &[u8], bool), on_extract_package_installer: fn(*mut (), Task::Id, DependencyID, &mut bun_install::ExtractData, Options::LogLevel), on_extract_store_installer: fn(*mut (), Task::Id), @@ -163,19 +168,30 @@ impl ErasedCallbacks { on_package_manifest_error: |c, name, err, url| { C::on_package_manifest_error(ctx::(c), name, err, url) }, - on_package_download_error_store: |c, task_id, name, resolution, err, url| { - C::on_package_download_error_store(ctx::(c), task_id, name, resolution, err, url) - }, - on_package_download_error_pkg: |c, package_id, name, resolution, err, url| { - C::on_package_download_error_pkg( - ctx::(c), - package_id, - name, - resolution, - err, - url, - ) - }, + on_package_download_error_store: + |c, task_id, name, resolution, err, url, is_required| { + C::on_package_download_error_store( + ctx::(c), + task_id, + name, + resolution, + err, + url, + is_required, + ) + }, + on_package_download_error_pkg: + |c, package_id, name, resolution, err, url, is_required| { + C::on_package_download_error_pkg( + ctx::(c), + package_id, + name, + resolution, + err, + url, + is_required, + ) + }, on_extract_package_installer: |c, task_id, dependency_id, data, log_level| { C::on_extract_package_installer( ctx::(c), @@ -785,6 +801,23 @@ fn run_tasks_erased( let is_required = manager.is_network_task_required(task.task_id); manager.mark_network_task_failed(task.task_id); + // A download that only optional dependencies need is a + // warning for both linkers. The callback reports an error + // itself, so only the warning is logged before it. + if !is_required { + bun_ast::add_warning_pretty!( + manager.log_mut(), + None, + bun_ast::Loc::EMPTY, + "{} downloading tarball {}@{}", + DownloadFailure(err.name(), &task.response), + bstr::BStr::new(extract.name.slice()), + extract + .resolution + .fmt(&manager.lockfile.buffers.string_bytes, PathSep::Auto,), + ); + } + if cb.has_on_package_download_error { if cb.is_store_installer { (cb.on_package_download_error_store)( @@ -794,6 +827,7 @@ fn run_tasks_erased( &extract.resolution, err, &task.url_buf, + is_required, ); } else { let package_id = manager.lockfile.buffers.resolutions @@ -805,6 +839,7 @@ fn run_tasks_erased( &extract.resolution, err, &task.url_buf, + is_required, ); } continue; @@ -822,18 +857,6 @@ fn run_tasks_erased( .resolution .fmt(&manager.lockfile.buffers.string_bytes, PathSep::Auto,), ); - } else { - bun_ast::add_warning_pretty!( - manager.log_mut(), - None, - bun_ast::Loc::EMPTY, - "{} downloading tarball {}@{}", - DownloadFailure(err.name(), &task.response), - bstr::BStr::new(extract.name.slice()), - extract - .resolution - .fmt(&manager.lockfile.buffers.string_bytes, PathSep::Auto,), - ); } fail_update_requests( manager, @@ -860,6 +883,17 @@ fn run_tasks_erased( let is_required = manager.is_network_task_required(task.task_id); manager.mark_network_task_failed(task.task_id); + if !is_required { + bun_ast::add_warning_pretty!( + manager.log_mut(), + None, + bun_ast::Loc::EMPTY, + "GET {} - {}", + bstr::BStr::new(metadata.url.slice()), + response.status_code, + ); + } + if cb.has_on_package_download_error { let err = match response.status_code { 400 => crate::Error::TarballHTTP400, @@ -879,6 +913,7 @@ fn run_tasks_erased( &extract.resolution, err, &task.url_buf, + is_required, ); } else { let package_id = manager.lockfile.buffers.resolutions @@ -890,6 +925,7 @@ fn run_tasks_erased( &extract.resolution, err, &task.url_buf, + is_required, ); } continue; @@ -904,15 +940,6 @@ fn run_tasks_erased( bstr::BStr::new(metadata.url.slice()), response.status_code, ); - } else { - bun_ast::add_warning_pretty!( - manager.log_mut(), - None, - bun_ast::Loc::EMPTY, - "GET {} - {}", - bstr::BStr::new(metadata.url.slice()), - response.status_code, - ); } fail_update_requests( manager, @@ -1166,6 +1193,7 @@ fn run_tasks_erased( resolution, err, fail_url, + true, ); } else { (cb.on_package_download_error_pkg)( @@ -1175,6 +1203,7 @@ fn run_tasks_erased( resolution, err, fail_url, + true, ); } continue; @@ -1365,6 +1394,7 @@ fn run_tasks_erased( res, err, url, + true, ); } } @@ -1386,6 +1416,7 @@ fn run_tasks_erased( &clone.res, err, url, + true, ); } } else if log_level != Options::LogLevel::Silent { @@ -1554,6 +1585,7 @@ fn run_tasks_erased( resolution, err, manager.lockfile.str(repo), + true, ); } else { bun_ast::add_error_pretty!( diff --git a/src/install/isolated_install.rs b/src/install/isolated_install.rs index 65128085d4be..a8934af05e0a 100644 --- a/src/install/isolated_install.rs +++ b/src/install/isolated_install.rs @@ -155,8 +155,9 @@ impl<'a> run_tasks::RunTasksCallbacks for StoreRunTasksCallbacks<'a> { resolution: &Resolution, err: crate::Error, url: &[u8], + is_required: bool, ) { - ctx.on_package_download_error(id, name, resolution, err, url); + ctx.on_package_download_error(id, name, resolution, err, url, is_required); } } @@ -2409,6 +2410,7 @@ pub(crate) fn install_isolated_packages( let ctx = install::TaskCallbackContext::IsolatedPackageInstallContext(entry_id); let dep = &lockfile_ro.buffers.dependencies[dep_id as usize]; + let is_required = dep.behavior.is_required(); match pkg_res_tag { ResolutionTag::Npm => { @@ -2429,12 +2431,7 @@ pub(crate) fn install_isolated_packages( crate::network_task::ForTarballError::AlreadyFailed | crate::network_task::ForTarballError::Offline, ) => { - // .monotonic is okay because an error means the task isn't - // running on another thread. - entry_steps[entry_id.get() as usize] - .store(installer::Step::Done as u32, Ordering::Relaxed); - installer - .on_task_complete(entry_id, installer::CompleteState::Fail); + installer.on_download_not_queued(entry_id, is_required); continue; } Err(err) => { @@ -2471,10 +2468,7 @@ pub(crate) fn install_isolated_packages( ) == crate::package_manager::GitEnqueueResult::OfflineMiss { // --offline and not cached: nothing was queued - entry_steps[entry_id.get() as usize] - .store(installer::Step::Done as u32, Ordering::Relaxed); - installer - .on_task_complete(entry_id, installer::CompleteState::Fail); + installer.on_download_not_queued(entry_id, is_required); continue; } } @@ -2499,12 +2493,7 @@ pub(crate) fn install_isolated_packages( crate::network_task::ForTarballError::AlreadyFailed | crate::network_task::ForTarballError::Offline, ) => { - // .monotonic is okay because an error means the task isn't - // running on another thread. - entry_steps[entry_id.get() as usize] - .store(installer::Step::Done as u32, Ordering::Relaxed); - installer - .on_task_complete(entry_id, installer::CompleteState::Fail); + installer.on_download_not_queued(entry_id, is_required); continue; } Err(err) => { @@ -2555,12 +2544,7 @@ pub(crate) fn install_isolated_packages( crate::network_task::ForTarballError::AlreadyFailed | crate::network_task::ForTarballError::Offline, ) => { - // .monotonic is okay because an error means the task isn't - // running on another thread. - entry_steps[entry_id.get() as usize] - .store(installer::Step::Done as u32, Ordering::Relaxed); - installer - .on_task_complete(entry_id, installer::CompleteState::Fail); + installer.on_download_not_queued(entry_id, is_required); continue; } Err(err) => { diff --git a/src/install/isolated_install/Installer.rs b/src/install/isolated_install/Installer.rs index 1ee1715dbf53..b6a17be39a96 100644 --- a/src/install/isolated_install/Installer.rs +++ b/src/install/isolated_install/Installer.rs @@ -240,6 +240,10 @@ impl<'a> Installer<'a> { /// Without this, the upfront pending-task slot for each waiting entry is /// never released and the install loop blocks forever on /// `pendingTaskCount() == 0`. + /// + /// `run_tasks` has already reported a download that only optional + /// dependencies need (`is_required == false`) as a warning. The waiting + /// entries are left out and the install does not fail. pub(crate) fn on_package_download_error( &mut self, task_id: crate::package_manager_task::Id, @@ -247,18 +251,34 @@ impl<'a> Installer<'a> { resolution: &Resolution, err: crate::Error, url: &[u8], + is_required: bool, ) { - if let Some(removed) = self.manager_mut().task_queue.remove(&task_id) { - let callbacks = removed; + let Some(callbacks) = self.manager_mut().task_queue.remove(&task_id) else { + if is_required { + // No waiting entry — still surface the error so it isn't lost. + let string_buf = self.lockfile().buffers.string_bytes.as_slice(); + Output::err_generic( + "failed to download {}@{}: {}\n {}", + ( + bstr::BStr::new(name), + resolution.fmt(string_buf, bun_core::fmt::PathSep::Auto), + bstr::BStr::new(download_error_reason(err)), + bstr::BStr::new(url), + ), + ); + Output::flush(); + } + return; + }; - let entry_steps = self.store.entries.items_step(); - for install_ctx in callbacks.as_slice() { - // `TaskCallbackContext` is an enum, so destructure. - let &TaskCallbackContext::IsolatedPackageInstallContext(entry_id) = install_ctx - else { - continue; - }; - entry_steps[entry_id.get() as usize].store(Step::Done as u32, Ordering::Relaxed); + let entry_steps = self.store.entries.items_step(); + for install_ctx in callbacks.as_slice() { + // `TaskCallbackContext` is an enum, so destructure. + let &TaskCallbackContext::IsolatedPackageInstallContext(entry_id) = install_ctx else { + continue; + }; + entry_steps[entry_id.get() as usize].store(Step::Done as u32, Ordering::Relaxed); + if is_required { self.on_task_fail( entry_id, &TaskError::Download(DownloadError { @@ -266,22 +286,11 @@ impl<'a> Installer<'a> { url: url.into(), }), ); + } else { + self.on_optional_download_fail(entry_id); } - // callbacks dropped here - } else { - // No waiting entry — still surface the error so it isn't lost. - let string_buf = self.lockfile().buffers.string_bytes.as_slice(); - Output::err_generic( - "failed to download {}@{}: {}\n {}", - ( - bstr::BStr::new(name), - resolution.fmt(string_buf, bun_core::fmt::PathSep::Auto), - bstr::BStr::new(download_error_reason(err)), - bstr::BStr::new(url), - ), - ); - Output::flush(); } + // callbacks dropped here } pub(crate) fn apply_package_patch( @@ -444,6 +453,32 @@ impl<'a> Installer<'a> { self.resume_unblocked_tasks(entry_id); } + /// Called from main thread when the download for `entry_id` was not + /// queued: it already failed, or `--offline` has no cached copy. Both + /// were reported when they happened. + pub(crate) fn on_download_not_queued(&mut self, entry_id: StoreEntryId, is_required: bool) { + // .monotonic is okay because the task isn't running on another thread. + self.store.entries.items_step()[entry_id.get() as usize] + .store(Step::Done as u32, Ordering::Relaxed); + if is_required { + self.on_task_complete(entry_id, CompleteState::Fail); + } else { + self.on_optional_download_fail(entry_id); + } + } + + /// Called from main thread when the download for an entry that only + /// optional dependencies need fails. The entry is left out. It counts + /// neither as installed nor as failed, as in the hoisted linker. + pub(crate) fn on_optional_download_fail(&mut self, entry_id: StoreEntryId) { + self.decrement_pending_tasks(); + self.resume_unblocked_tasks(entry_id); + + if let Some(node) = self.install_node.as_mut() { + node.complete_one(); + } + } + pub(crate) fn decrement_pending_tasks(&mut self) { self.manager_mut().decrement_pending_tasks(); } diff --git a/src/jsc/AsyncModule.rs b/src/jsc/AsyncModule.rs index 3db53bf12564..2d06271c36c5 100644 --- a/src/jsc/AsyncModule.rs +++ b/src/jsc/AsyncModule.rs @@ -262,6 +262,7 @@ impl run_tasks::RunTasksCallbacks for QueueRunTasksCallbac resolution: &Resolution, err: bun_install::Error, url: &[u8], + _is_required: bool, ) { ctx.on_package_download_error(package_id, name, resolution, err.name(), url) } diff --git a/test/cli/install/bun-install-retry.test.ts b/test/cli/install/bun-install-retry.test.ts index d9706157a9be..18823e71b63d 100644 --- a/test/cli/install/bun-install-retry.test.ts +++ b/test/cli/install/bun-install-retry.test.ts @@ -433,9 +433,81 @@ describe.each(["hoisted", "isolated"])("linker=%s", linker => { name: "baz", version: "0.0.3", }); - if (linker === "hoisted") { - expect(out).not.toContain("Failed to install"); - expect(exitCode).toBe(0); + expect(out).not.toContain("Failed to install"); + expect(exitCode).toBe(0); + }); + + // From a lockfile the install phase asks for the tarball first, so the + // failure reaches the linker instead of the resolver. + it("warns and exits 0 when an optional dependency's tarball fails to download from a lockfile", async () => { + const urls: string[] = []; + setHandler(async request => { + const { pathname } = new URL(request.url); + urls.push(pathname); + if (pathname === "/BaR") { + return Response.json({ + name: "BaR", + "dist-tags": { latest: "0.0.2" }, + versions: { + "0.0.2": { name: "BaR", version: "0.0.2", dist: { tarball: `${root_url}/BaR-0.0.2.tgz` } }, + }, + }); + } + if (pathname === "/BaR-0.0.2.tgz") return new Response("no", { status: 404 }); + return new Response("unexpected", { status: 404 }); + }); + await writeFile( + join(package_dir, "bunfig.toml"), + Bun.TOML.stringify({ + install: { + cache: false, + registry: `${root_url}/`, + linker, + }, + }), + ); + await writeFile( + join(package_dir, "package.json"), + JSON.stringify({ + name: "foo", + version: "0.0.1", + optionalDependencies: { BaR: "0.0.2" }, + }), + ); + async function install() { + await using proc = spawn({ + cmd: [bunExe(), "install", "--no-progress", "--ignore-scripts"], + cwd: package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([proc.stderr.text(), proc.stdout.text(), proc.exited]); + return { + warnLines: err.split("\n").filter(l => l.startsWith("warn:")), + errorLines: err.split("\n").filter(l => l.startsWith("error:")), + failed: out.includes("Failed to install"), + exitCode, + }; } + + // The first install resolves BaR and saves the lockfile. Its tarball fails while resolving. + expect(await install()).toEqual({ + warnLines: [`warn: GET ${root_url}/BaR-0.0.2.tgz - 404`], + errorLines: [], + failed: false, + exitCode: 0, + }); + await access(join(package_dir, "bun.lock")); + + urls.length = 0; + expect(await install()).toEqual({ + warnLines: [`warn: GET ${root_url}/BaR-0.0.2.tgz - 404`], + errorLines: [], + failed: false, + exitCode: 0, + }); + expect(urls).toEqual(["/BaR-0.0.2.tgz"]); }); }); From 586c6f6f3e9adc8a84ea1ac47728644f2cb2bb81 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 18 Sep 2026 03:49:24 +0000 Subject: [PATCH 02/10] install: an entry with a required edge fails, remove the links to a left-out entry, fail the update request --- src/install/PackageManager/runTasks.rs | 26 ++-- src/install/isolated_install.rs | 15 ++- src/install/isolated_install/Installer.rs | 138 ++++++++++++++++++--- src/install/isolated_install/Symlinker.rs | 40 +++--- test/cli/install/bun-install-retry.test.ts | 38 ++++-- 5 files changed, 194 insertions(+), 63 deletions(-) diff --git a/src/install/PackageManager/runTasks.rs b/src/install/PackageManager/runTasks.rs index 67c3b5fd5a3f..f55b11d59674 100644 --- a/src/install/PackageManager/runTasks.rs +++ b/src/install/PackageManager/runTasks.rs @@ -818,6 +818,13 @@ fn run_tasks_erased( ); } + fail_update_requests( + manager, + task.task_id, + extract.name.slice(), + Some(extract.dependency_id), + ); + if cb.has_on_package_download_error { if cb.is_store_installer { (cb.on_package_download_error_store)( @@ -858,12 +865,6 @@ fn run_tasks_erased( .fmt(&manager.lockfile.buffers.string_bytes, PathSep::Auto,), ); } - fail_update_requests( - manager, - task.task_id, - extract.name.slice(), - Some(extract.dependency_id), - ); if let Some(removed) = manager.task_queue.remove(&task.task_id) { drop(removed); @@ -894,6 +895,13 @@ fn run_tasks_erased( ); } + fail_update_requests( + manager, + task.task_id, + extract.name.slice(), + Some(extract.dependency_id), + ); + if cb.has_on_package_download_error { let err = match response.status_code { 400 => crate::Error::TarballHTTP400, @@ -941,12 +949,6 @@ fn run_tasks_erased( response.status_code, ); } - fail_update_requests( - manager, - task.task_id, - extract.name.slice(), - Some(extract.dependency_id), - ); if let Some(removed) = manager.task_queue.remove(&task.task_id) { drop(removed); diff --git a/src/install/isolated_install.rs b/src/install/isolated_install.rs index a8934af05e0a..6a56f5ea5856 100644 --- a/src/install/isolated_install.rs +++ b/src/install/isolated_install.rs @@ -2042,6 +2042,10 @@ pub(crate) fn install_isolated_packages( manager: manager_ptr, command_ctx, installed, + missing: (0..store.entries.len()) + .map(|_| std::sync::atomic::AtomicBool::new(false)) + .collect(), + required_entries: None, install_node: if show_progress { Some(&mut install_node) } else { @@ -2410,7 +2414,6 @@ pub(crate) fn install_isolated_packages( let ctx = install::TaskCallbackContext::IsolatedPackageInstallContext(entry_id); let dep = &lockfile_ro.buffers.dependencies[dep_id as usize]; - let is_required = dep.behavior.is_required(); match pkg_res_tag { ResolutionTag::Npm => { @@ -2431,7 +2434,7 @@ pub(crate) fn install_isolated_packages( crate::network_task::ForTarballError::AlreadyFailed | crate::network_task::ForTarballError::Offline, ) => { - installer.on_download_not_queued(entry_id, is_required); + installer.on_download_not_queued(entry_id); continue; } Err(err) => { @@ -2468,7 +2471,7 @@ pub(crate) fn install_isolated_packages( ) == crate::package_manager::GitEnqueueResult::OfflineMiss { // --offline and not cached: nothing was queued - installer.on_download_not_queued(entry_id, is_required); + installer.on_download_not_queued(entry_id); continue; } } @@ -2493,7 +2496,7 @@ pub(crate) fn install_isolated_packages( crate::network_task::ForTarballError::AlreadyFailed | crate::network_task::ForTarballError::Offline, ) => { - installer.on_download_not_queued(entry_id, is_required); + installer.on_download_not_queued(entry_id); continue; } Err(err) => { @@ -2544,7 +2547,7 @@ pub(crate) fn install_isolated_packages( crate::network_task::ForTarballError::AlreadyFailed | crate::network_task::ForTarballError::Offline, ) => { - installer.on_download_not_queued(entry_id, is_required); + installer.on_download_not_queued(entry_id); continue; } Err(err) => { @@ -2662,6 +2665,8 @@ pub(crate) fn install_isolated_packages( debug_assert!(done); } + installer.unlink_missing_dependencies(); + let mut summary = core::mem::take(&mut installer.summary); summary.successfully_installed = Some(core::mem::take(&mut installer.installed)); diff --git a/src/install/isolated_install/Installer.rs b/src/install/isolated_install/Installer.rs index b6a17be39a96..03dbf95df9c1 100644 --- a/src/install/isolated_install/Installer.rs +++ b/src/install/isolated_install/Installer.rs @@ -1,4 +1,4 @@ -use core::sync::atomic::{AtomicU8, Ordering}; +use core::sync::atomic::{AtomicBool, AtomicU8, Ordering}; use std::io::Write as _; use bun_ast::Log; @@ -75,6 +75,14 @@ pub struct Installer<'a> { pub(crate) summary: InstallSummary, pub(crate) installed: Bitset, + /// Set for an entry whose download failed and that only optional + /// dependencies need. Its store directory does not exist, so the entries + /// that depend on it do not link it. Set on the main thread before the + /// entry reaches `Step::Done`, read by the dependents' tasks. + pub(crate) missing: Box<[AtomicBool]>, + /// Main thread only. Set for an entry that a dependency without + /// `Behavior::OPTIONAL` resolves to. Built on the first failed download. + pub(crate) required_entries: Option, pub(crate) install_node: Option<&'a mut ProgressNode>, pub(crate) is_new_bun_modules: bool, @@ -277,8 +285,8 @@ impl<'a> Installer<'a> { let &TaskCallbackContext::IsolatedPackageInstallContext(entry_id) = install_ctx else { continue; }; - entry_steps[entry_id.get() as usize].store(Step::Done as u32, Ordering::Relaxed); - if is_required { + if is_required || self.entry_is_required(entry_id) { + entry_steps[entry_id.get() as usize].store(Step::Done as u32, Ordering::Relaxed); self.on_task_fail( entry_id, &TaskError::Download(DownloadError { @@ -456,11 +464,11 @@ impl<'a> Installer<'a> { /// Called from main thread when the download for `entry_id` was not /// queued: it already failed, or `--offline` has no cached copy. Both /// were reported when they happened. - pub(crate) fn on_download_not_queued(&mut self, entry_id: StoreEntryId, is_required: bool) { - // .monotonic is okay because the task isn't running on another thread. - self.store.entries.items_step()[entry_id.get() as usize] - .store(Step::Done as u32, Ordering::Relaxed); - if is_required { + pub(crate) fn on_download_not_queued(&mut self, entry_id: StoreEntryId) { + if self.entry_is_required(entry_id) { + // .monotonic is okay because the task isn't running on another thread. + self.store.entries.items_step()[entry_id.get() as usize] + .store(Step::Done as u32, Ordering::Relaxed); self.on_task_complete(entry_id, CompleteState::Fail); } else { self.on_optional_download_fail(entry_id); @@ -470,7 +478,12 @@ impl<'a> Installer<'a> { /// Called from main thread when the download for an entry that only /// optional dependencies need fails. The entry is left out. It counts /// neither as installed nor as failed, as in the hoisted linker. - pub(crate) fn on_optional_download_fail(&mut self, entry_id: StoreEntryId) { + fn on_optional_download_fail(&mut self, entry_id: StoreEntryId) { + self.missing[entry_id.get() as usize].store(true, Ordering::Relaxed); + // Release orders the `missing` store before the dependents' tasks, which + // load the step with Acquire in `is_task_blocked`. + self.store.entries.items_step()[entry_id.get() as usize] + .store(Step::Done as u32, Ordering::Release); self.decrement_pending_tasks(); self.resume_unblocked_tasks(entry_id); @@ -479,6 +492,28 @@ impl<'a> Installer<'a> { } } + /// Main thread only. Whether a dependency without `Behavior::OPTIONAL` + /// resolves to `entry_id`. The network task's `is_required` only knows the + /// one dependency that asked for the download. Several dependencies can + /// share one store entry, and the first one to reach it can be optional. + fn entry_is_required(&mut self, entry_id: StoreEntryId) -> bool { + if self.required_entries.is_none() { + let dependencies = self.lockfile().buffers.dependencies.as_slice(); + let mut required = bun_core::handle_oom(Bitset::init_empty(self.store.entries.len())); + for deps in self.store.entries.items_dependencies() { + for dep in deps.slice() { + if dependencies[dep.dep_id as usize].behavior.is_required() { + required.set(dep.entry_id.get() as usize); + } + } + } + self.required_entries = Some(required); + } + self.required_entries + .as_ref() + .is_some_and(|required| required.is_set(entry_id.get() as usize)) + } + pub(crate) fn decrement_pending_tasks(&mut self) { self.manager_mut().decrement_pending_tasks(); } @@ -664,6 +699,22 @@ pub enum Relink { Changed, } +/// Appends the `node_modules` entry name of a dependency link. A dependency +/// with the same name as the entry itself nests one `node_modules` deeper to +/// avoid the collision. +fn append_dependency_link_name( + dest: &mut AutoPath, + dep_name: &[u8], + entry_node_modules_name: Option<&[u8]>, +) { + // OOM/capacity: fire-and-forget + let _ = dest.append(dep_name); + if entry_node_modules_name.is_some_and(|name| strings::eql_long(dep_name, name, true)) { + let _ = dest.append(b"node_modules"); + let _ = dest.append(dep_name); + } +} + fn download_error_reason(e: crate::Error) -> &'static [u8] { match e { crate::Error::TarballHTTP400 => b"400 Bad Request", @@ -2296,15 +2347,13 @@ impl<'a> Installer<'a> { let mut changed = false; for dep in self.store.entries.items_dependencies()[entry_id.get() as usize].slice() { + if self.missing[dep.entry_id.get() as usize].load(Ordering::Relaxed) { + continue; + } let dep_name = dependencies[dep.dep_id as usize].name.slice(string_buf); dest.set_length(base_len); - let _ = dest.append(dep_name); // OOM/capacity: fire-and-forget - if entry_node_modules_name.is_some_and(|name| strings::eql_long(dep_name, name, true)) { - // same name as the entry itself: nest one node_modules deeper to avoid the collision - let _ = dest.append(b"node_modules"); // OOM/capacity: fire-and-forget - let _ = dest.append(dep_name); // OOM/capacity: fire-and-forget - } + append_dependency_link_name(&mut dest, dep_name, entry_node_modules_name); let mut dep_store_path = AutoAbsPath::init_top_level_dir(); if uses_global_store { @@ -2333,6 +2382,62 @@ impl<'a> Installer<'a> { Ok(changed) } + /// Main thread, after every task finished. Removes the links to the + /// entries in `missing`. `symlink_dependencies` runs before the entry's + /// download can fail, so the dependents created them. A dependent in the + /// global store keeps its link: that directory is shared, and the link + /// resolves once another install downloads the entry. + pub(crate) fn unlink_missing_dependencies(&self) { + if !self + .missing + .iter() + .any(|missing| missing.load(Ordering::Relaxed)) + { + return; + } + + let lockfile = self.lockfile(); + let string_buf = lockfile.buffers.string_bytes.as_slice(); + let dependencies = lockfile.buffers.dependencies.as_slice(); + let pkg_names = lockfile.packages.items_name(); + let pkg_resolutions = lockfile.packages.items_resolution(); + let entry_node_ids = self.store.entries.items_node_id(); + let node_pkg_ids = self.store.nodes.items_pkg_id(); + let node_dep_ids = self.store.nodes.items_dep_id(); + + for (parent, deps) in self.store.entries.items_dependencies().iter().enumerate() { + let parent_id = StoreEntryId::from(parent as u32); + if self.entry_uses_global_store(parent_id) { + continue; + } + let mut dest: Option<(AutoPath, usize, Option<&[u8]>)> = None; + for dep in deps.slice() { + if !self.missing[dep.entry_id.get() as usize].load(Ordering::Relaxed) { + continue; + } + let (dest, base_len, entry_node_modules_name) = dest.get_or_insert_with(|| { + let node_id = entry_node_ids[parent]; + let pkg_id = node_pkg_ids[node_id.get() as usize]; + let dep_id = node_dep_ids[node_id.get() as usize]; + let mut dest = AutoPath::init_top_level_dir(); + self.append_real_store_node_modules_path(&mut dest, parent_id, Which::Final); + let base_len = dest.len(); + let name = self.entry_store_node_modules_package_name( + dep_id, + pkg_id, + &pkg_resolutions[pkg_id as usize], + pkg_names, + ); + (dest, base_len, name) + }); + let dep_name = dependencies[dep.dep_id as usize].name.slice(string_buf); + dest.set_length(*base_len); + append_dependency_link_name(dest, dep_name, *entry_node_modules_name); + symlinker::remove_link(dest.slice_z()); + } + } + } + pub(crate) fn link_dependency_bins(&self, parent_entry_id: StoreEntryId) -> crate::Result<()> { let lockfile = self.lockfile(); let store = self.store; @@ -2369,6 +2474,9 @@ impl<'a> Installer<'a> { ); for dep in entry_deps[parent_entry_id.get() as usize].slice() { + if self.missing[dep.entry_id.get() as usize].load(Ordering::Relaxed) { + continue; + } let node_id = entry_node_ids[dep.entry_id.get() as usize]; let dep_id = node_dep_ids[node_id.get() as usize]; let pkg_id = node_pkg_ids[node_id.get() as usize]; diff --git a/src/install/isolated_install/Symlinker.rs b/src/install/isolated_install/Symlinker.rs index 52c90735abba..6f977b00828e 100644 --- a/src/install/isolated_install/Symlinker.rs +++ b/src/install/isolated_install/Symlinker.rs @@ -123,32 +123,36 @@ impl Symlinker { { return Ok(false); } - - // this existing link is pointing to the wrong package. - // on windows rmdir must be used for symlinks created to point - // at directories, even if the target no longer exists - match bun_sys::rmdir(self.dest.slice_z()) { - Ok(()) => {} - Err(err) => match err.get_errno() { - Errno::EPERM => { - let _ = bun_sys::unlink(self.dest.slice_z()); - } - _ => {} - }, - } - } - #[cfg(not(windows))] - { - // this existing link is pointing to the wrong package - let _ = bun_sys::unlink(self.dest.slice_z()); } + // this existing link is pointing to the wrong package + remove_link(self.dest.slice_z()); + return self.symlink().map(|()| true); } } } } +/// Removes the link at `dest`. Errors are ignored: the caller writes a new link +/// or does not need the old one. +pub(crate) fn remove_link(dest: &bun_core::ZStr) { + #[cfg(windows)] + { + // on windows rmdir must be used for symlinks created to point + // at directories, even if the target no longer exists + if let Err(err) = bun_sys::rmdir(dest) { + if matches!(err.get_errno(), Errno::EPERM) { + let _ = bun_sys::unlink(dest); + } + } + } + #[cfg(not(windows))] + { + let _ = bun_sys::unlink(dest); + } +} + #[derive(Clone, Copy)] pub enum Strategy { ExpectExisting, diff --git a/test/cli/install/bun-install-retry.test.ts b/test/cli/install/bun-install-retry.test.ts index 18823e71b63d..77e06ddaf44f 100644 --- a/test/cli/install/bun-install-retry.test.ts +++ b/test/cli/install/bun-install-retry.test.ts @@ -1,5 +1,6 @@ import { file, spawn } from "bun"; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, setDefaultTimeout } from "bun:test"; +import { lstatSync } from "fs"; import { access, writeFile } from "fs/promises"; import { bunExe, bunEnv as env, readdirSorted, tmpdirSync, toBeValidBin, toBeWorkspaceLink, toHaveBins } from "harness"; import { join } from "path"; @@ -15,6 +16,16 @@ import { setHandler, } from "./dummy.registry"; +/** Whether `path` exists, as a directory, a file, or a link (even a dangling one). */ +function entryExists(path: string): boolean { + try { + lstatSync(path); + return true; + } catch { + return false; + } +} + beforeAll(dummyBeforeAll); afterAll(dummyAfterAll); @@ -474,9 +485,9 @@ describe.each(["hoisted", "isolated"])("linker=%s", linker => { optionalDependencies: { BaR: "0.0.2" }, }), ); - async function install() { + async function install(...args: string[]) { await using proc = spawn({ - cmd: [bunExe(), "install", "--no-progress", "--ignore-scripts"], + cmd: [bunExe(), ...args, "--no-progress", "--ignore-scripts"], cwd: package_dir, stdout: "pipe", stdin: "pipe", @@ -488,26 +499,27 @@ describe.each(["hoisted", "isolated"])("linker=%s", linker => { warnLines: err.split("\n").filter(l => l.startsWith("warn:")), errorLines: err.split("\n").filter(l => l.startsWith("error:")), failed: out.includes("Failed to install"), + // The package is left out. No dangling link to it either. + linked: entryExists(join(package_dir, "node_modules", "BaR")), exitCode, }; } - - // The first install resolves BaR and saves the lockfile. Its tarball fails while resolving. - expect(await install()).toEqual({ + const warned = { warnLines: [`warn: GET ${root_url}/BaR-0.0.2.tgz - 404`], errorLines: [], failed: false, - exitCode: 0, - }); + linked: false, + }; + + // The first install resolves BaR and saves the lockfile. Its tarball fails while resolving. + expect(await install("install")).toEqual({ ...warned, exitCode: 0 }); await access(join(package_dir, "bun.lock")); urls.length = 0; - expect(await install()).toEqual({ - warnLines: [`warn: GET ${root_url}/BaR-0.0.2.tgz - 404`], - errorLines: [], - failed: false, - exitCode: 0, - }); + expect(await install("install")).toEqual({ ...warned, exitCode: 0 }); expect(urls).toEqual(["/BaR-0.0.2.tgz"]); + + // A package that the user asked for fails the command, optional or not. + expect(await install("add", "--optional", "BaR@0.0.2")).toEqual({ ...warned, exitCode: 1 }); }); }); From 6914032593802daa6a51d305ffaf7dfb25ab279c Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 18 Sep 2026 03:51:02 +0000 Subject: [PATCH 03/10] install: shorten the comments --- src/install/PackageManager/runTasks.rs | 7 +--- src/install/isolated_install/Installer.rs | 39 ++++++----------------- src/install/isolated_install/Symlinker.rs | 6 ++-- 3 files changed, 12 insertions(+), 40 deletions(-) diff --git a/src/install/PackageManager/runTasks.rs b/src/install/PackageManager/runTasks.rs index f55b11d59674..80a1c5a14b55 100644 --- a/src/install/PackageManager/runTasks.rs +++ b/src/install/PackageManager/runTasks.rs @@ -80,9 +80,7 @@ pub trait RunTasksCallbacks { // `package_id: PackageID` otherwise. Static dispatch via two trait // methods lets impls receive the correctly-typed id without a // `Task::Id` round-trip pun. - // - // `is_required` is false when only optional dependencies need the - // download. `run_tasks` has then already logged the failure as a warning. + // `is_required == false`: only optional dependencies need the download. fn on_package_download_error_store( _ctx: &mut Self::Ctx, _task_id: Task::Id, @@ -801,9 +799,6 @@ fn run_tasks_erased( let is_required = manager.is_network_task_required(task.task_id); manager.mark_network_task_failed(task.task_id); - // A download that only optional dependencies need is a - // warning for both linkers. The callback reports an error - // itself, so only the warning is logged before it. if !is_required { bun_ast::add_warning_pretty!( manager.log_mut(), diff --git a/src/install/isolated_install/Installer.rs b/src/install/isolated_install/Installer.rs index 03dbf95df9c1..6d5e86f2a3a2 100644 --- a/src/install/isolated_install/Installer.rs +++ b/src/install/isolated_install/Installer.rs @@ -75,13 +75,9 @@ pub struct Installer<'a> { pub(crate) summary: InstallSummary, pub(crate) installed: Bitset, - /// Set for an entry whose download failed and that only optional - /// dependencies need. Its store directory does not exist, so the entries - /// that depend on it do not link it. Set on the main thread before the - /// entry reaches `Step::Done`, read by the dependents' tasks. + /// Entries left out because their download failed and only optional dependencies need them. pub(crate) missing: Box<[AtomicBool]>, - /// Main thread only. Set for an entry that a dependency without - /// `Behavior::OPTIONAL` resolves to. Built on the first failed download. + /// Main thread only. Entries that a dependency without `Behavior::OPTIONAL` resolves to. pub(crate) required_entries: Option, pub(crate) install_node: Option<&'a mut ProgressNode>, pub(crate) is_new_bun_modules: bool, @@ -248,10 +244,6 @@ impl<'a> Installer<'a> { /// Without this, the upfront pending-task slot for each waiting entry is /// never released and the install loop blocks forever on /// `pendingTaskCount() == 0`. - /// - /// `run_tasks` has already reported a download that only optional - /// dependencies need (`is_required == false`) as a warning. The waiting - /// entries are left out and the install does not fail. pub(crate) fn on_package_download_error( &mut self, task_id: crate::package_manager_task::Id, @@ -461,9 +453,7 @@ impl<'a> Installer<'a> { self.resume_unblocked_tasks(entry_id); } - /// Called from main thread when the download for `entry_id` was not - /// queued: it already failed, or `--offline` has no cached copy. Both - /// were reported when they happened. + /// Called from main thread when the download for `entry_id` already failed or `--offline` has no cached copy. pub(crate) fn on_download_not_queued(&mut self, entry_id: StoreEntryId) { if self.entry_is_required(entry_id) { // .monotonic is okay because the task isn't running on another thread. @@ -475,13 +465,10 @@ impl<'a> Installer<'a> { } } - /// Called from main thread when the download for an entry that only - /// optional dependencies need fails. The entry is left out. It counts - /// neither as installed nor as failed, as in the hoisted linker. + /// Called from main thread. The entry is left out, neither installed nor failed. fn on_optional_download_fail(&mut self, entry_id: StoreEntryId) { self.missing[entry_id.get() as usize].store(true, Ordering::Relaxed); - // Release orders the `missing` store before the dependents' tasks, which - // load the step with Acquire in `is_task_blocked`. + // Release pairs with the Acquire load in `is_task_blocked`. self.store.entries.items_step()[entry_id.get() as usize] .store(Step::Done as u32, Ordering::Release); self.decrement_pending_tasks(); @@ -492,10 +479,7 @@ impl<'a> Installer<'a> { } } - /// Main thread only. Whether a dependency without `Behavior::OPTIONAL` - /// resolves to `entry_id`. The network task's `is_required` only knows the - /// one dependency that asked for the download. Several dependencies can - /// share one store entry, and the first one to reach it can be optional. + /// Main thread only. Whether a dependency without `Behavior::OPTIONAL` resolves to `entry_id`. fn entry_is_required(&mut self, entry_id: StoreEntryId) -> bool { if self.required_entries.is_none() { let dependencies = self.lockfile().buffers.dependencies.as_slice(); @@ -699,9 +683,7 @@ pub enum Relink { Changed, } -/// Appends the `node_modules` entry name of a dependency link. A dependency -/// with the same name as the entry itself nests one `node_modules` deeper to -/// avoid the collision. +/// A dependency with the same name as the entry itself nests one `node_modules` deeper. fn append_dependency_link_name( dest: &mut AutoPath, dep_name: &[u8], @@ -2382,11 +2364,7 @@ impl<'a> Installer<'a> { Ok(changed) } - /// Main thread, after every task finished. Removes the links to the - /// entries in `missing`. `symlink_dependencies` runs before the entry's - /// download can fail, so the dependents created them. A dependent in the - /// global store keeps its link: that directory is shared, and the link - /// resolves once another install downloads the entry. + /// Main thread, after every task finished: `symlink_dependencies` runs before a download can fail. pub(crate) fn unlink_missing_dependencies(&self) { if !self .missing @@ -2407,6 +2385,7 @@ impl<'a> Installer<'a> { for (parent, deps) in self.store.entries.items_dependencies().iter().enumerate() { let parent_id = StoreEntryId::from(parent as u32); + // A global store entry is shared: the link resolves once another install downloads the entry. if self.entry_uses_global_store(parent_id) { continue; } diff --git a/src/install/isolated_install/Symlinker.rs b/src/install/isolated_install/Symlinker.rs index 6f977b00828e..6bbe48ff0773 100644 --- a/src/install/isolated_install/Symlinker.rs +++ b/src/install/isolated_install/Symlinker.rs @@ -134,13 +134,11 @@ impl Symlinker { } } -/// Removes the link at `dest`. Errors are ignored: the caller writes a new link -/// or does not need the old one. +/// Removes the link at `dest`. Errors are ignored. pub(crate) fn remove_link(dest: &bun_core::ZStr) { #[cfg(windows)] { - // on windows rmdir must be used for symlinks created to point - // at directories, even if the target no longer exists + // a directory symlink needs rmdir, even if the target no longer exists if let Err(err) = bun_sys::rmdir(dest) { if matches!(err.get_errno(), Errno::EPERM) { let _ = bun_sys::unlink(dest); From 6e77914873b0f2245822be901ecf90edcb1106d8 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 18 Sep 2026 04:10:25 +0000 Subject: [PATCH 04/10] install: keep the dependency links of a global store entry, an optional peer edge does not require an entry --- src/install/isolated_install/Installer.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/install/isolated_install/Installer.rs b/src/install/isolated_install/Installer.rs index 6d5e86f2a3a2..06f739b3578d 100644 --- a/src/install/isolated_install/Installer.rs +++ b/src/install/isolated_install/Installer.rs @@ -486,7 +486,8 @@ impl<'a> Installer<'a> { let mut required = bun_core::handle_oom(Bitset::init_empty(self.store.entries.len())); for deps in self.store.entries.items_dependencies() { for dep in deps.slice() { - if dependencies[dep.dep_id as usize].behavior.is_required() { + let behavior = dependencies[dep.dep_id as usize].behavior; + if !behavior.is_optional() && !behavior.is_optional_peer() { required.set(dep.entry_id.get() as usize); } } @@ -2329,7 +2330,10 @@ impl<'a> Installer<'a> { let mut changed = false; for dep in self.store.entries.items_dependencies()[entry_id.get() as usize].slice() { - if self.missing[dep.entry_id.get() as usize].load(Ordering::Relaxed) { + // A global store entry is shared: the link resolves once another install downloads the entry. + if !uses_global_store + && self.missing[dep.entry_id.get() as usize].load(Ordering::Relaxed) + { continue; } let dep_name = dependencies[dep.dep_id as usize].name.slice(string_buf); @@ -2453,9 +2457,6 @@ impl<'a> Installer<'a> { ); for dep in entry_deps[parent_entry_id.get() as usize].slice() { - if self.missing[dep.entry_id.get() as usize].load(Ordering::Relaxed) { - continue; - } let node_id = entry_node_ids[dep.entry_id.get() as usize]; let dep_id = node_dep_ids[node_id.get() as usize]; let pkg_id = node_pkg_ids[node_id.get() as usize]; From 2c35fb47df0805e2ec82be7ddd1e0464e0c67409 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 18 Sep 2026 04:20:41 +0000 Subject: [PATCH 05/10] install: remove the links to a left-out entry before its dependents resume --- src/install/isolated_install/Installer.rs | 60 +++++++++++------------ 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/src/install/isolated_install/Installer.rs b/src/install/isolated_install/Installer.rs index 06f739b3578d..5fbb28505df5 100644 --- a/src/install/isolated_install/Installer.rs +++ b/src/install/isolated_install/Installer.rs @@ -468,6 +468,8 @@ impl<'a> Installer<'a> { /// Called from main thread. The entry is left out, neither installed nor failed. fn on_optional_download_fail(&mut self, entry_id: StoreEntryId) { self.missing[entry_id.get() as usize].store(true, Ordering::Relaxed); + // The dependents that wait on this entry made their links before they blocked. + self.unlink_links_to(entry_id); // Release pairs with the Acquire load in `is_task_blocked`. self.store.entries.items_step()[entry_id.get() as usize] .store(Step::Done as u32, Ordering::Release); @@ -2370,52 +2372,50 @@ impl<'a> Installer<'a> { /// Main thread, after every task finished: `symlink_dependencies` runs before a download can fail. pub(crate) fn unlink_missing_dependencies(&self) { - if !self - .missing - .iter() - .any(|missing| missing.load(Ordering::Relaxed)) - { - return; + for (entry, missing) in self.missing.iter().enumerate() { + if missing.load(Ordering::Relaxed) { + self.unlink_links_to(StoreEntryId::from(entry as u32)); + } } + } + /// Main thread. Removes the links that the dependents outside the global store made to `entry_id`. + fn unlink_links_to(&self, entry_id: StoreEntryId) { let lockfile = self.lockfile(); let string_buf = lockfile.buffers.string_bytes.as_slice(); let dependencies = lockfile.buffers.dependencies.as_slice(); let pkg_names = lockfile.packages.items_name(); let pkg_resolutions = lockfile.packages.items_resolution(); - let entry_node_ids = self.store.entries.items_node_id(); + let entries = &self.store.entries; + let entry_node_ids = entries.items_node_id(); + let entry_deps = entries.items_dependencies(); let node_pkg_ids = self.store.nodes.items_pkg_id(); let node_dep_ids = self.store.nodes.items_dep_id(); - for (parent, deps) in self.store.entries.items_dependencies().iter().enumerate() { - let parent_id = StoreEntryId::from(parent as u32); + for &parent_id in entries.items_parents()[entry_id.get() as usize].as_slice() { // A global store entry is shared: the link resolves once another install downloads the entry. - if self.entry_uses_global_store(parent_id) { + if parent_id == StoreEntryId::INVALID || self.entry_uses_global_store(parent_id) { continue; } - let mut dest: Option<(AutoPath, usize, Option<&[u8]>)> = None; - for dep in deps.slice() { - if !self.missing[dep.entry_id.get() as usize].load(Ordering::Relaxed) { + let node_id = entry_node_ids[parent_id.get() as usize]; + let pkg_id = node_pkg_ids[node_id.get() as usize]; + let dep_id = node_dep_ids[node_id.get() as usize]; + let mut dest = AutoPath::init_top_level_dir(); + self.append_real_store_node_modules_path(&mut dest, parent_id, Which::Final); + let base_len = dest.len(); + let entry_node_modules_name = self.entry_store_node_modules_package_name( + dep_id, + pkg_id, + &pkg_resolutions[pkg_id as usize], + pkg_names, + ); + for dep in entry_deps[parent_id.get() as usize].slice() { + if dep.entry_id != entry_id { continue; } - let (dest, base_len, entry_node_modules_name) = dest.get_or_insert_with(|| { - let node_id = entry_node_ids[parent]; - let pkg_id = node_pkg_ids[node_id.get() as usize]; - let dep_id = node_dep_ids[node_id.get() as usize]; - let mut dest = AutoPath::init_top_level_dir(); - self.append_real_store_node_modules_path(&mut dest, parent_id, Which::Final); - let base_len = dest.len(); - let name = self.entry_store_node_modules_package_name( - dep_id, - pkg_id, - &pkg_resolutions[pkg_id as usize], - pkg_names, - ); - (dest, base_len, name) - }); let dep_name = dependencies[dep.dep_id as usize].name.slice(string_buf); - dest.set_length(*base_len); - append_dependency_link_name(dest, dep_name, *entry_node_modules_name); + dest.set_length(base_len); + append_dependency_link_name(&mut dest, dep_name, entry_node_modules_name); symlinker::remove_link(dest.slice_z()); } } From 6ceed9b35a43edbae176336ff3cd1073352b7e70 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 18 Sep 2026 04:40:52 +0000 Subject: [PATCH 06/10] install: remove the links to a left-out entry from the entries that link each install --- src/install/isolated_install/Installer.rs | 63 +++++++++++++-------- test/cli/install/bun-install-retry.test.ts | 66 ++++++++++++++++++++++ 2 files changed, 105 insertions(+), 24 deletions(-) diff --git a/src/install/isolated_install/Installer.rs b/src/install/isolated_install/Installer.rs index 5fbb28505df5..b7b5ba78c0c6 100644 --- a/src/install/isolated_install/Installer.rs +++ b/src/install/isolated_install/Installer.rs @@ -2330,12 +2330,10 @@ impl<'a> Installer<'a> { self.append_real_store_node_modules_path(&mut dest, entry_id, Which::Staging); let base_len = dest.len(); + let skips_missing = self.links_each_install(entry_id); let mut changed = false; for dep in self.store.entries.items_dependencies()[entry_id.get() as usize].slice() { - // A global store entry is shared: the link resolves once another install downloads the entry. - if !uses_global_store - && self.missing[dep.entry_id.get() as usize].load(Ordering::Relaxed) - { + if skips_missing && self.missing[dep.entry_id.get() as usize].load(Ordering::Relaxed) { continue; } let dep_name = dependencies[dep.dep_id as usize].name.slice(string_buf); @@ -2379,7 +2377,20 @@ impl<'a> Installer<'a> { } } - /// Main thread. Removes the links that the dependents outside the global store made to `entry_id`. + /// Whether the entry writes its dependency links on every install. A store entry relinks + /// instead: a link that it misses counts as a change, and that would run its scripts again. + /// A global store entry is shared: its link resolves once another install downloads the entry. + fn links_each_install(&self, entry_id: StoreEntryId) -> bool { + let node_id = self.store.entries.items_node_id()[entry_id.get() as usize]; + let pkg_id = self.store.nodes.items_pkg_id()[node_id.get() as usize]; + let tag = self.lockfile().packages.items_resolution()[pkg_id as usize].tag; + matches!( + tag, + ResolutionTag::Root | ResolutionTag::Workspace | ResolutionTag::Folder + ) + } + + /// Main thread. Removes the links to `entry_id` from the entries that link each install. fn unlink_links_to(&self, entry_id: StoreEntryId) { let lockfile = self.lockfile(); let string_buf = lockfile.buffers.string_bytes.as_slice(); @@ -2388,34 +2399,38 @@ impl<'a> Installer<'a> { let pkg_resolutions = lockfile.packages.items_resolution(); let entries = &self.store.entries; let entry_node_ids = entries.items_node_id(); - let entry_deps = entries.items_dependencies(); let node_pkg_ids = self.store.nodes.items_pkg_id(); let node_dep_ids = self.store.nodes.items_dep_id(); - for &parent_id in entries.items_parents()[entry_id.get() as usize].as_slice() { - // A global store entry is shared: the link resolves once another install downloads the entry. - if parent_id == StoreEntryId::INVALID || self.entry_uses_global_store(parent_id) { + // Not `items_parents()`: a public hoist adds the entry to the root's list only. + for (parent, deps) in entries.items_dependencies().iter().enumerate() { + let parent_id = StoreEntryId::from(parent as u32); + if !self.links_each_install(parent_id) { continue; } - let node_id = entry_node_ids[parent_id.get() as usize]; - let pkg_id = node_pkg_ids[node_id.get() as usize]; - let dep_id = node_dep_ids[node_id.get() as usize]; - let mut dest = AutoPath::init_top_level_dir(); - self.append_real_store_node_modules_path(&mut dest, parent_id, Which::Final); - let base_len = dest.len(); - let entry_node_modules_name = self.entry_store_node_modules_package_name( - dep_id, - pkg_id, - &pkg_resolutions[pkg_id as usize], - pkg_names, - ); - for dep in entry_deps[parent_id.get() as usize].slice() { + let mut dest: Option<(AutoPath, usize, Option<&[u8]>)> = None; + for dep in deps.slice() { if dep.entry_id != entry_id { continue; } + let (dest, base_len, entry_node_modules_name) = dest.get_or_insert_with(|| { + let node_id = entry_node_ids[parent]; + let pkg_id = node_pkg_ids[node_id.get() as usize]; + let dep_id = node_dep_ids[node_id.get() as usize]; + let mut dest = AutoPath::init_top_level_dir(); + self.append_real_store_node_modules_path(&mut dest, parent_id, Which::Final); + let base_len = dest.len(); + let name = self.entry_store_node_modules_package_name( + dep_id, + pkg_id, + &pkg_resolutions[pkg_id as usize], + pkg_names, + ); + (dest, base_len, name) + }); let dep_name = dependencies[dep.dep_id as usize].name.slice(string_buf); - dest.set_length(base_len); - append_dependency_link_name(&mut dest, dep_name, entry_node_modules_name); + dest.set_length(*base_len); + append_dependency_link_name(dest, dep_name, *entry_node_modules_name); symlinker::remove_link(dest.slice_z()); } } diff --git a/test/cli/install/bun-install-retry.test.ts b/test/cli/install/bun-install-retry.test.ts index 77e06ddaf44f..c5f991bb971c 100644 --- a/test/cli/install/bun-install-retry.test.ts +++ b/test/cli/install/bun-install-retry.test.ts @@ -522,4 +522,70 @@ describe.each(["hoisted", "isolated"])("linker=%s", linker => { // A package that the user asked for fails the command, optional or not. expect(await install("add", "--optional", "BaR@0.0.2")).toEqual({ ...warned, exitCode: 1 }); }); + + // The dependent is a store entry in the isolated linker. It keeps its links across installs, so + // a repeat install reports no changes. + it("a dependency whose optional dependency's tarball fails installs once", async () => { + const tarball = await new Bun.Archive( + { + "package/package.json": JSON.stringify({ + name: "pkg", + version: "1.0.0", + optionalDependencies: { BaR: "0.0.2" }, + }), + }, + { compress: "gzip" }, + ).bytes(); + setHandler(async request => { + const { pathname } = new URL(request.url); + const manifest = (name: string, version: string, extra: object) => + Response.json({ + name, + "dist-tags": { latest: version }, + versions: { [version]: { name, version, ...extra, dist: { tarball: `${root_url}/${name}-${version}.tgz` } } }, + }); + if (pathname === "/BaR") return manifest("BaR", "0.0.2", {}); + if (pathname === "/pkg") return manifest("pkg", "1.0.0", { optionalDependencies: { BaR: "0.0.2" } }); + if (pathname === "/pkg-1.0.0.tgz") return new Response(tarball); + return new Response("no", { status: 404 }); + }); + await writeFile( + join(package_dir, "bunfig.toml"), + Bun.TOML.stringify({ install: { cache: false, registry: `${root_url}/`, linker } }), + ); + await writeFile( + join(package_dir, "package.json"), + JSON.stringify({ name: "foo", version: "0.0.1", dependencies: { pkg: "1.0.0" } }), + ); + async function install() { + await using proc = spawn({ + cmd: [bunExe(), "install", "--no-progress", "--ignore-scripts"], + cwd: package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([proc.stderr.text(), proc.stdout.text(), proc.exited]); + return { + warnLines: err.split("\n").filter(l => l.startsWith("warn:")), + errorLines: err.split("\n").filter(l => l.startsWith("error:")), + summary: out.includes("no changes") ? "no changes" : out.includes("installed") ? "installed" : out, + linked: entryExists(join(package_dir, "node_modules", "BaR")), + exitCode, + }; + } + const warned = { + warnLines: [`warn: GET ${root_url}/BaR-0.0.2.tgz - 404`], + errorLines: [], + linked: false, + exitCode: 0, + }; + expect(await install()).toEqual({ ...warned, summary: "installed" }); + expect(await install()).toEqual({ ...warned, summary: "no changes" }); + expect(await file(join(package_dir, "node_modules", "pkg", "package.json")).json()).toMatchObject({ + name: "pkg", + version: "1.0.0", + }); + }); }); From d05e47065185c06d0588e2a99621d56287cf4d6a Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 18 Sep 2026 04:41:23 +0000 Subject: [PATCH 07/10] install: shorten a comment --- src/install/isolated_install/Installer.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/install/isolated_install/Installer.rs b/src/install/isolated_install/Installer.rs index b7b5ba78c0c6..72be2044d80e 100644 --- a/src/install/isolated_install/Installer.rs +++ b/src/install/isolated_install/Installer.rs @@ -2377,9 +2377,7 @@ impl<'a> Installer<'a> { } } - /// Whether the entry writes its dependency links on every install. A store entry relinks - /// instead: a link that it misses counts as a change, and that would run its scripts again. - /// A global store entry is shared: its link resolves once another install downloads the entry. + /// A store entry relinks instead: a link it misses is a change, and that runs its scripts again. fn links_each_install(&self, entry_id: StoreEntryId) -> bool { let node_id = self.store.entries.items_node_id()[entry_id.get() as usize]; let pkg_id = self.store.nodes.items_pkg_id()[node_id.get() as usize]; From 6573ed06d7802e6e5cbbdf4920cf1275c9451629 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 18 Sep 2026 04:50:57 +0000 Subject: [PATCH 08/10] ci: retrigger From 494fa34b741911bd8d4ead70652894d7be635e8b Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 8 Oct 2026 02:55:16 +0000 Subject: [PATCH 09/10] install: a package that any dependency of the store requires fails, the store's edges keep the first dependency only --- src/install/isolated_install.rs | 2 +- src/install/isolated_install/Installer.rs | 42 ++++++++---- test/cli/install/bun-install-retry.test.ts | 76 ++++++++++++++++++++++ 3 files changed, 105 insertions(+), 15 deletions(-) diff --git a/src/install/isolated_install.rs b/src/install/isolated_install.rs index 6a56f5ea5856..3ccf997c6c87 100644 --- a/src/install/isolated_install.rs +++ b/src/install/isolated_install.rs @@ -2045,7 +2045,7 @@ pub(crate) fn install_isolated_packages( missing: (0..store.entries.len()) .map(|_| std::sync::atomic::AtomicBool::new(false)) .collect(), - required_entries: None, + required_packages: None, install_node: if show_progress { Some(&mut install_node) } else { diff --git a/src/install/isolated_install/Installer.rs b/src/install/isolated_install/Installer.rs index 72be2044d80e..f7fda0dd1ac9 100644 --- a/src/install/isolated_install/Installer.rs +++ b/src/install/isolated_install/Installer.rs @@ -23,6 +23,7 @@ use crate::resolution; use crate::{ self as install, DependencyID, Lockfile, PackageID, PackageManager, PackageNameHash, Resolution, TaskCallbackContext, TruncatedPackageNameHash, bin, invalid_dependency_id, + invalid_package_id, }; // Bring `items_()` column accessors into scope for // `MultiArrayList` / `Slice`. @@ -77,8 +78,8 @@ pub struct Installer<'a> { pub(crate) installed: Bitset, /// Entries left out because their download failed and only optional dependencies need them. pub(crate) missing: Box<[AtomicBool]>, - /// Main thread only. Entries that a dependency without `Behavior::OPTIONAL` resolves to. - pub(crate) required_entries: Option, + /// Main thread only. Packages that a dependency without `Behavior::OPTIONAL` resolves to. + pub(crate) required_packages: Option, pub(crate) install_node: Option<&'a mut ProgressNode>, pub(crate) is_new_bun_modules: bool, @@ -481,24 +482,37 @@ impl<'a> Installer<'a> { } } - /// Main thread only. Whether a dependency without `Behavior::OPTIONAL` resolves to `entry_id`. + /// Main thread only. Whether a dependency without `Behavior::OPTIONAL` of a package in the + /// store resolves to the package of `entry_id`. The store's own edges do not tell: a deduped + /// node keeps the dependency that reached it first, which can be the optional one. fn entry_is_required(&mut self, entry_id: StoreEntryId) -> bool { - if self.required_entries.is_none() { - let dependencies = self.lockfile().buffers.dependencies.as_slice(); - let mut required = bun_core::handle_oom(Bitset::init_empty(self.store.entries.len())); - for deps in self.store.entries.items_dependencies() { - for dep in deps.slice() { - let behavior = dependencies[dep.dep_id as usize].behavior; - if !behavior.is_optional() && !behavior.is_optional_peer() { - required.set(dep.entry_id.get() as usize); + let lockfile = self.lockfile(); + let node_pkg_ids = self.store.nodes.items_pkg_id(); + let entry_node_ids = self.store.entries.items_node_id(); + if self.required_packages.is_none() { + let dependencies = lockfile.buffers.dependencies.as_slice(); + let resolutions = lockfile.buffers.resolutions.as_slice(); + let pkg_deps = lockfile.packages.items_dependencies(); + let mut required = bun_core::handle_oom(Bitset::init_empty(lockfile.packages.len())); + for &node_id in entry_node_ids { + let list = pkg_deps[node_pkg_ids[node_id.get() as usize] as usize]; + for dep_id in list.off..list.off + list.len { + let behavior = dependencies[dep_id as usize].behavior; + let res = resolutions[dep_id as usize]; + if res != invalid_package_id + && !behavior.is_optional() + && !behavior.is_optional_peer() + { + required.set(res as usize); } } } - self.required_entries = Some(required); + self.required_packages = Some(required); } - self.required_entries + let pkg_id = node_pkg_ids[entry_node_ids[entry_id.get() as usize].get() as usize]; + self.required_packages .as_ref() - .is_some_and(|required| required.is_set(entry_id.get() as usize)) + .is_some_and(|required| required.is_set(pkg_id as usize)) } pub(crate) fn decrement_pending_tasks(&mut self) { diff --git a/test/cli/install/bun-install-retry.test.ts b/test/cli/install/bun-install-retry.test.ts index c5f991bb971c..eb9e7338fd85 100644 --- a/test/cli/install/bun-install-retry.test.ts +++ b/test/cli/install/bun-install-retry.test.ts @@ -588,4 +588,80 @@ describe.each(["hoisted", "isolated"])("linker=%s", linker => { version: "1.0.0", }); }); + + // One package is optional for one dependent and required by another. Its failed download + // must fail the install whichever dependent reaches it first. The hoisted linker has the + // same hole on main; #43197 closes it for both linkers. + describe.skipIf(linker === "hoisted")("a failed download that an optional and a required dependency share", () => { + async function project(root: object, packages: Record) { + const tarballs = new Map(); + for (const [name, pkg] of Object.entries(packages)) { + tarballs.set( + name, + await new Bun.Archive( + { "package/package.json": JSON.stringify({ name, version: "1.0.0", ...pkg }) }, + { compress: "gzip" }, + ).bytes(), + ); + } + setHandler(async request => { + const { pathname } = new URL(request.url); + const manifest = (name: string, version: string, extra: object) => + Response.json({ + name, + "dist-tags": { latest: version }, + versions: { + [version]: { name, version, ...extra, dist: { tarball: `${root_url}/${name}-${version}.tgz` } }, + }, + }); + if (pathname === "/BaR") return manifest("BaR", "0.0.2", {}); + const name = pathname.slice(1); + if (name in packages) return manifest(name, "1.0.0", packages[name]); + const tarball = tarballs.get(name.replace(/-1\.0\.0\.tgz$/, "")); + if (tarball && name.endsWith("-1.0.0.tgz")) return new Response(tarball); + return new Response("no", { status: 404 }); + }); + await writeFile( + join(package_dir, "bunfig.toml"), + Bun.TOML.stringify({ install: { cache: false, registry: `${root_url}/`, linker } }), + ); + await writeFile(join(package_dir, "package.json"), JSON.stringify({ name: "foo", version: "0.0.1", ...root })); + } + async function install() { + await using proc = spawn({ + cmd: [bunExe(), "install", "--no-progress", "--ignore-scripts"], + cwd: package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([proc.stderr.text(), proc.stdout.text(), proc.exited]); + return { + failed: out.includes("Failed to install") || err.split("\n").some(l => l.startsWith("error:")), + exitCode, + }; + } + + it("optional for the root, required by a dependency", async () => { + await project( + { dependencies: { needs: "1.0.0" }, optionalDependencies: { BaR: "0.0.2" } }, + { needs: { dependencies: { BaR: "0.0.2" } } }, + ); + expect(await install()).toEqual({ failed: true, exitCode: 1 }); + await access(join(package_dir, "bun.lock")); + expect(await install()).toEqual({ failed: true, exitCode: 1 }); + }); + + // `pkg` reaches BaR first in one order, the required dependent in the other. + it.each(["aneeds", "zneeds"])("optional for a dependency, required by %s", async required => { + await project( + { dependencies: { pkg: "1.0.0", [required]: "1.0.0" } }, + { pkg: { optionalDependencies: { BaR: "0.0.2" } }, [required]: { dependencies: { BaR: "0.0.2" } } }, + ); + expect(await install()).toEqual({ failed: true, exitCode: 1 }); + await access(join(package_dir, "bun.lock")); + expect(await install()).toEqual({ failed: true, exitCode: 1 }); + }); + }); }); From 263d09b2265abd7956c3e40f31a474c81a569ffd Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 8 Oct 2026 02:56:00 +0000 Subject: [PATCH 10/10] install: shorten a comment --- src/install/isolated_install/Installer.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/install/isolated_install/Installer.rs b/src/install/isolated_install/Installer.rs index f7fda0dd1ac9..ade5bdc382cf 100644 --- a/src/install/isolated_install/Installer.rs +++ b/src/install/isolated_install/Installer.rs @@ -482,9 +482,7 @@ impl<'a> Installer<'a> { } } - /// Main thread only. Whether a dependency without `Behavior::OPTIONAL` of a package in the - /// store resolves to the package of `entry_id`. The store's own edges do not tell: a deduped - /// node keeps the dependency that reached it first, which can be the optional one. + /// Main thread only. Reads the lockfile edges: a deduped store node keeps its first dependency only. fn entry_is_required(&mut self, entry_id: StoreEntryId) -> bool { let lockfile = self.lockfile(); let node_pkg_ids = self.store.nodes.items_pkg_id();