Skip to content

install: do not fail the isolated install when an optional dependency's tarball cannot be downloaded #43229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 10 commits into from
Closed

install: do not fail the isolated install when an optional dependency's tarball cannot be downloaded #43229

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1f6d2fd
install: do not fail the isolated install when an optional dependency…
robobun Sep 18, 2026
586c6f6
install: an entry with a required edge fails, remove the links to a l…
robobun Sep 18, 2026
6914032
install: shorten the comments
robobun Sep 18, 2026
6e77914
install: keep the dependency links of a global store entry, an option…
robobun Sep 18, 2026
2c35fb4
install: remove the links to a left-out entry before its dependents r…
robobun Sep 18, 2026
6ceed9b
install: remove the links to a left-out entry from the entries that l…
robobun Sep 18, 2026
d05e470
install: shorten a comment
robobun Sep 18, 2026
6573ed0
ci: retrigger
robobun Sep 18, 2026
494fa34
install: a package that any dependency of the store requires fails, t…
robobun Oct 8, 2026
263d09b
install: shorten a comment
robobun Oct 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 68 additions & 36 deletions src/install/PackageManager/runTasks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,17 @@ 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.
Comment thread
robobun marked this conversation as resolved.
Resolved
fn on_package_download_error_store(
_ctx: &mut Self::Ctx,
_task_id: Task::Id,
_name: &[u8],
_resolution: &bun_install::Resolution,
_err: crate::Error,
_url: &[u8],
_is_required: bool,
) {
unreachable!()
}
Expand All @@ -97,6 +101,7 @@ pub trait RunTasksCallbacks {
_resolution: &bun_install::Resolution,
_err: crate::Error,
_url: &[u8],
_is_required: bool,
) {
unreachable!()
}
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -163,19 +168,30 @@ impl ErasedCallbacks {
on_package_manifest_error: |c, name, err, url| {
C::on_package_manifest_error(ctx::<C>(c), name, err, url)
},
on_package_download_error_store: |c, task_id, name, resolution, err, url| {
C::on_package_download_error_store(ctx::<C>(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>(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>(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>(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>(c),
Expand Down Expand Up @@ -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.
Comment thread
robobun marked this conversation as resolved.
Resolved
if !is_required {
bun_ast::add_warning_pretty!(
manager.log_mut(),
None,
bun_ast::Loc::EMPTY,
"{} downloading tarball <b>{}@{}<r>",
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)(
Expand All @@ -794,6 +827,7 @@ fn run_tasks_erased(
&extract.resolution,
err,
&task.url_buf,
is_required,
);
} else {
let package_id = manager.lockfile.buffers.resolutions
Expand All @@ -805,6 +839,7 @@ fn run_tasks_erased(
&extract.resolution,
err,
&task.url_buf,
is_required,
);
}
continue;
Expand All @@ -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 <b>{}@{}<r>",
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,
Expand All @@ -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,
"<r><yellow><b>GET<r><yellow> {}<d> - {}<r>",
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,
Expand All @@ -879,6 +913,7 @@ fn run_tasks_erased(
&extract.resolution,
err,
&task.url_buf,
is_required,
);
} else {
let package_id = manager.lockfile.buffers.resolutions
Expand All @@ -890,6 +925,7 @@ fn run_tasks_erased(
&extract.resolution,
err,
&task.url_buf,
is_required,
);
}
continue;
Comment thread
robobun marked this conversation as resolved.
Resolved
Expand All @@ -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,
"<r><yellow><b>GET<r><yellow> {}<d> - {}<r>",
bstr::BStr::new(metadata.url.slice()),
response.status_code,
);
}
fail_update_requests(
manager,
Expand Down Expand Up @@ -1166,6 +1193,7 @@ fn run_tasks_erased(
resolution,
err,
fail_url,
true,
);
} else {
(cb.on_package_download_error_pkg)(
Expand All @@ -1175,6 +1203,7 @@ fn run_tasks_erased(
resolution,
err,
fail_url,
true,
);
}
continue;
Expand Down Expand Up @@ -1365,6 +1394,7 @@ fn run_tasks_erased(
res,
err,
url,
true,
);
}
}
Expand All @@ -1386,6 +1416,7 @@ fn run_tasks_erased(
&clone.res,
err,
url,
true,
);
}
} else if log_level != Options::LogLevel::Silent {
Expand Down Expand Up @@ -1554,6 +1585,7 @@ fn run_tasks_erased(
resolution,
err,
manager.lockfile.str(repo),
true,
);
} else {
bun_ast::add_error_pretty!(
Expand Down
30 changes: 7 additions & 23 deletions src/install/isolated_install.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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();
Comment thread
robobun marked this conversation as resolved.
Resolved

match pkg_res_tag {
ResolutionTag::Npm => {
Expand All @@ -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) => {
Expand Down Expand Up @@ -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);
Comment thread
robobun marked this conversation as resolved.
Resolved
continue;
}
}
Expand All @@ -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) => {
Expand Down Expand Up @@ -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) => {
Expand Down
Loading
Loading