Skip to content

share one run tasks body across install callbacks #39516

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
alii wants to merge 4 commits into from
Closed

share one run tasks body across install callbacks #39516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
664dea5
share one run tasks body across install callbacks
alii Aug 18, 2026
4213ca0
pass the callbacks object straight through process_dependency_list
robobun Aug 18, 2026
3d9bb2f
name the two bool arguments at the patch apply install call
robobun Aug 18, 2026
01efdf5
test that a failed tarball download reaches the store installer callback
robobun Aug 18, 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
66 changes: 33 additions & 33 deletions src/install/PackageInstaller.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -927,9 +927,9 @@ impl<'a> PackageInstaller<'a> {
self.current_tree_id = context.tree_id;

// Re-verify: a parent reinstall may have deleted a deferred entry.
const NEEDS_VERIFY: bool = true;
const IS_PENDING_PACKAGE_INSTALL: bool = true;
self.install_package_with_name_and_resolution::<NEEDS_VERIFY, IS_PENDING_PACKAGE_INSTALL>(
self.install_package_with_name_and_resolution(
/* needs_verify */ true,
/* is_pending_package_install */ true,
// This id might be different from the id used to enqueue the task. Important
// to use the correct one because the package might be aliased with a different
// name
Expand Down Expand Up @@ -1163,9 +1163,9 @@ impl<'a> PackageInstaller<'a> {
// `DependencyInstallContext.path: Vec<u8>` — clone since `cb` is `&`.
self.node_modules.path.clone_from(&context.path);
self.current_tree_id = context.tree_id;
const NEEDS_VERIFY: bool = false;
const IS_PENDING_PACKAGE_INSTALL: bool = false;
self.install_package_with_name_and_resolution::<NEEDS_VERIFY, IS_PENDING_PACKAGE_INSTALL>(
self.install_package_with_name_and_resolution(
/* needs_verify */ false,
/* is_pending_package_install */ false,
// This id might be different from the id used to enqueue the task. Important
// to use the correct one because the package might be aliased with a different
// name
Expand Down Expand Up @@ -1259,15 +1259,14 @@ impl<'a> PackageInstaller<'a> {
count
}

pub(crate) fn install_package_with_name_and_resolution<
pub(crate) fn install_package_with_name_and_resolution(
&mut self,
// false when coming from download. if the package was downloaded
// it was already determined to need an install
const NEEDS_VERIFY: bool,
needs_verify: bool,
// we don't want to allow more package installs through
// pending packages if we're already draining them.
const IS_PENDING_PACKAGE_INSTALL: bool,
>(
&mut self,
is_pending_package_install: bool,
dependency_id: DependencyID,
package_id: PackageID,
log_level: Options::LogLevel,
Expand Down Expand Up @@ -1300,7 +1299,7 @@ impl<'a> PackageInstaller<'a> {
}
self.summary.fail += 1;
self.increment_tree_install_count(
!IS_PENDING_PACKAGE_INSTALL,
!is_pending_package_install,
self.current_tree_id,
log_level,
);
Expand Down Expand Up @@ -1506,7 +1505,7 @@ impl<'a> PackageInstaller<'a> {
}
self.summary.fail += 1;
self.increment_tree_install_count(
!IS_PENDING_PACKAGE_INSTALL,
!is_pending_package_install,
self.current_tree_id,
log_level,
);
Expand Down Expand Up @@ -1589,7 +1588,7 @@ impl<'a> PackageInstaller<'a> {
panic!("Internal assertion failure: unexpected resolution tag");
}
self.increment_tree_install_count(
!IS_PENDING_PACKAGE_INSTALL,
!is_pending_package_install,
self.current_tree_id,
log_level,
);
Expand All @@ -1599,7 +1598,7 @@ impl<'a> PackageInstaller<'a> {

let needs_install = self.force_install
|| self.skip_verify_installed_version_number
|| !NEEDS_VERIFY
|| !needs_verify
|| remove_patch
|| !installer.verify(resolution, &self.root_node_modules_folder);

Expand Down Expand Up @@ -1653,11 +1652,11 @@ impl<'a> PackageInstaller<'a> {
Ok(()) => {}
Err(ForTarballError::OutOfMemory) => bun_core::out_of_memory(),
Err(ForTarballError::InvalidURL) => {
self.fail_with_invalid_url::<IS_PENDING_PACKAGE_INSTALL>(log_level)
self.fail_with_invalid_url(is_pending_package_install, log_level)
}
Err(ForTarballError::AlreadyFailed) => self
.increment_tree_install_count(
!IS_PENDING_PACKAGE_INSTALL,
!is_pending_package_install,
self.current_tree_id,
log_level,
),
Expand Down Expand Up @@ -1685,11 +1684,11 @@ impl<'a> PackageInstaller<'a> {
Ok(()) => {}
Err(ForTarballError::OutOfMemory) => bun_core::out_of_memory(),
Err(ForTarballError::InvalidURL) => {
self.fail_with_invalid_url::<IS_PENDING_PACKAGE_INSTALL>(log_level)
self.fail_with_invalid_url(is_pending_package_install, log_level)
}
Err(ForTarballError::AlreadyFailed) => self
.increment_tree_install_count(
!IS_PENDING_PACKAGE_INSTALL,
!is_pending_package_install,
self.current_tree_id,
log_level,
),
Expand Down Expand Up @@ -1723,11 +1722,11 @@ impl<'a> PackageInstaller<'a> {
Ok(()) => {}
Err(ForTarballError::OutOfMemory) => bun_core::out_of_memory(),
Err(ForTarballError::InvalidURL) => {
self.fail_with_invalid_url::<IS_PENDING_PACKAGE_INSTALL>(log_level)
self.fail_with_invalid_url(is_pending_package_install, log_level)
}
Err(ForTarballError::AlreadyFailed) => self
.increment_tree_install_count(
!IS_PENDING_PACKAGE_INSTALL,
!is_pending_package_install,
self.current_tree_id,
log_level,
),
Expand All @@ -1738,7 +1737,7 @@ impl<'a> PackageInstaller<'a> {
panic!("unreachable, handled above");
}
self.increment_tree_install_count(
!IS_PENDING_PACKAGE_INSTALL,
!is_pending_package_install,
self.current_tree_id,
log_level,
);
Expand Down Expand Up @@ -1771,7 +1770,7 @@ impl<'a> PackageInstaller<'a> {
}
}

if !IS_PENDING_PACKAGE_INSTALL
if !is_pending_package_install
&& !Self::can_install_package_for_tree(
&self.completed_trees,
self.lockfile().buffers.trees.as_slice(),
Expand Down Expand Up @@ -1810,7 +1809,7 @@ impl<'a> PackageInstaller<'a> {
}
self.summary.fail += 1;
self.increment_tree_install_count(
!IS_PENDING_PACKAGE_INSTALL,
!is_pending_package_install,
self.current_tree_id,
log_level,
);
Expand Down Expand Up @@ -2075,7 +2074,7 @@ impl<'a> PackageInstaller<'a> {
}

self.increment_tree_install_count(
!IS_PENDING_PACKAGE_INSTALL,
!is_pending_package_install,
self.current_tree_id,
log_level,
);
Expand All @@ -2090,7 +2089,7 @@ impl<'a> PackageInstaller<'a> {
// even if the package failed to install, we still need to increment the install
// counter for this tree
self.increment_tree_install_count(
!IS_PENDING_PACKAGE_INSTALL,
!is_pending_package_install,
self.current_tree_id,
log_level,
);
Expand Down Expand Up @@ -2222,7 +2221,7 @@ impl<'a> PackageInstaller<'a> {
} else {
// Same gate as the `needs_install` branch: a pending parent's
// `uninstall_before_install` would delete this verified package.
if !IS_PENDING_PACKAGE_INSTALL
if !is_pending_package_install
&& !Self::can_install_package_for_tree(
&self.completed_trees,
self.lockfile().buffers.trees.as_slice(),
Expand Down Expand Up @@ -2378,20 +2377,21 @@ impl<'a> PackageInstaller<'a> {
// only used in the `needs_install` branch's EACCES handler).
destination_dir.close();
self.increment_tree_install_count(
!IS_PENDING_PACKAGE_INSTALL,
!is_pending_package_install,
self.current_tree_id,
log_level,
);
}
}

fn fail_with_invalid_url<const IS_PENDING_PACKAGE_INSTALL: bool>(
fn fail_with_invalid_url(
&mut self,
is_pending_package_install: bool,
log_level: Options::LogLevel,
) {
self.summary.fail += 1;
self.increment_tree_install_count(
!IS_PENDING_PACKAGE_INSTALL,
!is_pending_package_install,
self.current_tree_id,
log_level,
);
Expand Down Expand Up @@ -2511,9 +2511,9 @@ impl<'a> PackageInstaller<'a> {
// `&mut self` call.
let resolutions = self.resolutions;

const NEEDS_VERIFY: bool = true;
const IS_PENDING_PACKAGE_INSTALL: bool = false;
self.install_package_with_name_and_resolution::<NEEDS_VERIFY, IS_PENDING_PACKAGE_INSTALL>(
self.install_package_with_name_and_resolution(
/* needs_verify */ true,
/* is_pending_package_install */ false,
dep_id,
package_id,
log_level,
Expand Down
16 changes: 7 additions & 9 deletions src/install/PackageManager/PackageManagerEnqueue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,12 +487,10 @@ pub fn enqueue_dependency_to_root(
// this callback, so this is the unique live borrow.
let manager = unsafe { &mut *self.manager };
if manager.pending_task_count() > 0 {
// All callbacks void: `VoidRunTasksCallbacks` (below)
// has `Ctx = ()` and every `HAS_* = false`.
let log_level = manager.options.log_level;
if let Err(err) = run_tasks::run_tasks::<VoidRunTasksCallbacks>(
if let Err(err) = run_tasks::run_tasks(
manager,
&mut (),
&mut VoidRunTasksCallbacks,
false,
log_level,
) {
Expand Down Expand Up @@ -554,12 +552,12 @@ pub fn enqueue_dependency_to_root(
}
}

/// All-void callback set used by `enqueueDependencyToRoot` and `runAndWaitFn`:
/// `Ctx = ()`, no callbacks, so the `HAS_*` const-gates compile out the
/// callback paths.
/// All-void callback set used by `enqueueDependencyToRoot`.
struct VoidRunTasksCallbacks;
impl run_tasks::RunTasksCallbacks for VoidRunTasksCallbacks {
type Ctx = ();
impl run_tasks::RunTasksCallbacks<'_> for VoidRunTasksCallbacks {
fn flags(&self) -> run_tasks::RunTasksFlags {
run_tasks::RunTasksFlags::default()
}
}

pub fn enqueue_network_task(this: &mut PackageManager, task: *mut NetworkTask) {
Expand Down
26 changes: 12 additions & 14 deletions src/install/PackageManager/PopulateManifestCache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::invalid_package_id;
// `runTasks.rs` / `PackageManagerEnqueue.rs`).
use super::PackageManager;
use super::enqueue;
use super::run_tasks::{self, RunTasksCallbacks};
use super::run_tasks::{self, RunTasksCallbacks, RunTasksFlags};
use crate::package_manager_task as Task;
use crate::resolution::Tag as ResolutionTag;

Expand Down Expand Up @@ -104,10 +104,14 @@ pub enum Packages<'a> {
/// `RunTasksCallbacks` impl for the void-callback `runTasks` call in
/// `populateManifestCache`.
struct ManifestsOnlyCallbacks;
impl RunTasksCallbacks for ManifestsOnlyCallbacks {
type Ctx = ();
const PROGRESS_BAR: bool = true;
const MANIFESTS_ONLY: bool = true;
impl RunTasksCallbacks<'_> for ManifestsOnlyCallbacks {
fn flags(&self) -> RunTasksFlags {
RunTasksFlags {
progress_bar: true,
manifests_only: true,
..Default::default()
}
}
}

/// Populate the manifest cache for packages included from `root_pkg_ids`. Only manifests of
Expand Down Expand Up @@ -320,15 +324,9 @@ pub fn populate_manifest_cache(
// callback, so this is the unique live borrow.
let manager = unsafe { &mut *closure.manager };
let log_level = manager.options.log_level;
// void RunTasksCallbacks — `extract_ctx` is unit. Do NOT pass
// `manager` as both receiver and ctx (aliased &mut); the generic
// context collapses to `&mut ()`.
if let Err(err) = run_tasks::run_tasks::<ManifestsOnlyCallbacks>(
manager,
&mut (),
true,
log_level,
) {
if let Err(err) =
run_tasks::run_tasks(manager, &mut ManifestsOnlyCallbacks, true, log_level)
{
closure.err = Some(err);
return true;
}
Expand Down
17 changes: 9 additions & 8 deletions src/install/PackageManager/install_with_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ use bun_install_types::NodeLinker::NodeLinker;

// Free-function "methods" on `PackageManager` hosted in sibling modules
// to avoid one giant `impl PackageManager` block.
use crate::package_manager_real::run_tasks::{RunTasksCallbacks, run_tasks};
use crate::package_manager_real::run_tasks::{RunTasksCallbacks, RunTasksFlags, run_tasks};
use crate::package_manager_real::{
UpdateRequest, enqueue_dependency_list, enqueue_dependency_with_main, enqueue_patch_task_pre,
save_lockfile, setup_global_dir, update_lockfile_if_needed, write_yarn_lock,
Expand Down Expand Up @@ -998,9 +998,13 @@ pub fn install_with_manager(
/// `runAndWaitFn::isDone` (no hooks, `progress_bar = true`). Only these
/// flags differ from the default.
struct InstallWaitCallbacks;
impl RunTasksCallbacks for InstallWaitCallbacks {
type Ctx = ();
const PROGRESS_BAR: bool = true;
impl RunTasksCallbacks<'_> for InstallWaitCallbacks {
fn flags(&self) -> RunTasksFlags {
RunTasksFlags {
progress_bar: true,
..Default::default()
}
}
}

struct RunAndWaitClosure<const CHECK_PEERS: bool, const ONLY_PRE_PATCH: bool> {
Expand Down Expand Up @@ -1034,11 +1038,8 @@ impl<const CHECK_PEERS: bool, const ONLY_PRE_PATCH: bool>

this.drain_dependency_list();

// void RunTasksCallbacks — the trait dispatch needs a
// concrete `RunTasksCallbacks` impl; `extract_ctx` collapses to `()` so we
// do NOT pass `this` as both receiver and ctx (would alias `&mut`).
let log_level = this.options.log_level;
if let Err(err) = run_tasks::<InstallWaitCallbacks>(this, &mut (), CHECK_PEERS, log_level) {
if let Err(err) = run_tasks(this, &mut InstallWaitCallbacks, CHECK_PEERS, log_level) {
closure.err = Some(err);
return true;
}
Expand Down
Loading
Loading