Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 17 additions & 0 deletions src/install/PackageInstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2337,8 +2337,25 @@ impl<'a> PackageInstall<'a> {
manager: &mut PackageManager,
package_id: PackageID,
resolution_tag: resolution::Tag,
force_refresh_tarball: bool,
) -> bool {
let state = manager.get_preinstall_state(package_id);
// `force_refresh_tarball` is set by the caller when a URL/local tarball
// must re-fetch its bytes (`--force`, or the dependency was explicitly
// named on the command line): the cache folder is keyed by the URL/path
// hash, so an unchanged key hides changed content behind a stale
// extraction. Report it as missing so a download/read task is enqueued.
//
// Gated on the state not already being `Done`: once this run has
// downloaded and extracted the tarball, its state is `Done` (set after
// extraction) and the cache is fresh, so the package must install from
// cache rather than re-enqueue into an already-drained/deduped task.
// Without the `Done` guard, a first-time `--force` install (no lockfile)
// where the resolve phase fetched the tarball would re-enqueue and
// silently skip installing it.
if force_refresh_tarball && state != crate::PreinstallState::Done {
return true;
}
match state {
crate::PreinstallState::Done => false,
_ => 'brk: {
Expand Down
29 changes: 28 additions & 1 deletion src/install/PackageInstaller.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1492,7 +1492,33 @@
}
}

let needs_install = self.force_install
// A URL/local tarball is re-fetched under `--force`, and also when the
// dependency was explicitly named on the command line (`bun i <url>`,
// `bun update <name>`): the bytes behind the same cache key may have
// changed. Only on the initial install-phase pass (`NEEDS_VERIFY`); the
// post-extraction callback installs from the freshly refreshed cache.
// Skipped when this run already fetched the tarball (the resolve phase
// re-downloads it when `bun update` invalidates the resolution) — the
// cache is fresh, so install from it instead of re-enqueueing into the
// already-drained task.
let force_refresh_tarball = NEEDS_VERIFY
&& resolution.tag.is_tarball_cache_keyed_by_url()
&& (self.force_install
|| self
.manager_mut()
.dependency_is_update_request(dependency_id))
&& {
let url = match resolution.tag {
resolution::Tag::RemoteTarball => {
resolution.remote_tarball().slice(string_buf!())
}
_ => resolution.local_tarball().slice(string_buf!()),
};
!self.manager_mut().tarball_task_enqueued_this_run(url)
};

Check warning on line 1518 in src/install/PackageInstaller.rs

View check run for this annotation

Claude / Claude Code Review

tarball_task_enqueued_this_run guard is redundant and causes partial refresh for multi-entry URL tarballs

The `!tarball_task_enqueued_this_run(url)` clause here (and its mirror in `isolated_install.rs:2373`) is redundant for its stated purpose — every "resolve phase already fetched it" case it guards against already has `PreinstallState::Done`, which `package_missing_from_cache` checks first. The only case where it fires distinctly is when an install-phase download for a *prior* entry of the same `package_id` is still in flight, and there it makes the second entry fall through to the stale on-disk c
Comment thread
robobun marked this conversation as resolved.

let needs_install = force_refresh_tarball
|| self.force_install
|| self.skip_verify_installed_version_number
|| !NEEDS_VERIFY
|| remove_patch
Expand All @@ -1505,6 +1531,7 @@
self.manager_mut(),
package_id,
resolution.tag,
force_refresh_tarball,
)
{
if cfg!(debug_assertions) {
Expand Down
12 changes: 11 additions & 1 deletion src/install/PackageManager/PackageManagerEnqueue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,17 @@ pub fn enqueue_tarball_for_reading(
return;
}

let integrity = this.lockfile.packages.items_meta()[package_id as usize].integrity;
// Under `--force`, or when the dependency was explicitly named on the
// command line, the tarball at this path may have changed, so drop the
// lockfile-pinned integrity: `ExtractTarball::run` recomputes it from the
// fresh bytes instead of rejecting them. See `enqueue_tarball_for_download`.
let integrity = if resolution.tag.is_tarball_cache_keyed_by_url()
&& (this.options.enable.force_install() || this.dependency_is_update_request(dependency_id))
{
Integrity::default()
} else {
this.lockfile.packages.items_meta()[package_id as usize].integrity
};

let task = enqueue_local_tarball(
this,
Expand Down
37 changes: 36 additions & 1 deletion src/install/PackageManager/PackageManagerLifecycle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@ use crate::lifecycle_script_runner::{
};
use crate::lockfile_real::package::scripts::List as ScriptsList;
use crate::package_manager_real::Command;
use crate::package_manager_task as PmTask;
use crate::resolution_real::Tag as ResolutionTag;
use bun_install::lockfile::{self, Lockfile, Package};
use bun_install::{
PackageID, PackageManager, PreinstallState, TruncatedPackageNameHash, invalid_package_id,
DependencyID, PackageID, PackageManager, PreinstallState, TruncatedPackageNameHash,
invalid_package_id,
};

#[derive(Default)]
Expand Down Expand Up @@ -533,6 +535,39 @@ impl PackageManager {

set
}

/// Whether `dependency_id` was explicitly named on the command line
/// (`bun add <pkg>` / `bun install <pkg-or-url>` / `bun update <pkg>`).
/// URL/path arguments produce unnamed requests that match on the
/// dependency's version literal.
pub fn dependency_is_update_request(&self, dependency_id: DependencyID) -> bool {
if self.update_requests.is_empty() {
return false;
}
// `dependency_id` can be `invalid_dependency_id` (e.g. a root entry).
let Some(dep) = self
.lockfile
.buffers
.dependencies
.get(dependency_id as usize)
else {
return false;
};
let string_buf = self.lockfile.buffers.string_bytes.as_slice();
self.update_requests
.iter()
.any(|request| request.matches(dep, string_buf))
}

/// Whether a fetch/read task for this tarball URL/path was already
/// enqueued during this run (e.g. the resolve phase re-downloaded it
/// because `bun update` invalidated the resolution). The extract success
/// path drains the task's callback list but leaves the key in
/// `task_queue`, so the key's presence means the bytes were already
/// refreshed; re-enqueueing would push a callback nothing ever drains.
pub fn tarball_task_enqueued_this_run(&self, url: &[u8]) -> bool {
self.task_queue.contains(&PmTask::Id::for_tarball(url))
}
}

fn add_dependencies_to_set(
Expand Down
47 changes: 46 additions & 1 deletion src/install/PackageManager/runTasks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use bun_http::{self as http, AsyncHTTP};
use bun_threading::thread_pool::Batch as ThreadPoolBatch;

use crate::extract_tarball;
use crate::integrity::Integrity;
use crate::network_task::Callback as NetworkTaskCallback;
use crate::npm;
use crate::patch_install::{Callback as PatchTaskCallback, PatchTask};
Expand Down Expand Up @@ -1143,6 +1144,35 @@ pub fn run_tasks<C: RunTasksCallbacks>(
bun_core::analytics::Features::extracted_packages_inc();

if C::HAS_ON_EXTRACT {
// When a re-fetch of a URL/local tarball was requested
// (`--force`, or the dependency was explicitly named on the
// command line), the bytes may have changed, so
// `ExtractTarball::run` recomputed the integrity from the fresh
// bytes (the stored pin was dropped before download). Persist it
// over the stale lockfile hash; otherwise a later cache-cleared
// install would reject the new content against the old pin. This
// is the install-phase path (hoisted + isolated), where
// `package_id` is already the final mapping and neither installer
// otherwise rewrites an already-resolved package's hash. The
// resolve phase below goes through
// `process_extracted_tarball_package`, which records the
// integrity on the final package itself.
if package_id != INVALID_PACKAGE_ID
&& resolution.tag.is_tarball_cache_keyed_by_url()
&& (manager.options.enable.force_install()
|| manager.dependency_is_update_request(dependency_id))
{
let new_integrity = task.data_extract().integrity;
if new_integrity.tag.is_supported() {
manager.lockfile.packages.items_meta_mut()[package_id as usize]
.integrity = new_integrity;
manager
.options
.enable
.set(Enable::FORCE_SAVE_LOCKFILE, true);
}
}

if C::IS_PACKAGE_INSTALLER {
C::as_package_installer(extract_ctx).fix_cached_lockfile_package_slices();
C::on_extract_package_installer(
Expand Down Expand Up @@ -1804,6 +1834,21 @@ pub fn generate_network_task_for_tarball<'a>(
// so the task's drop never closes them.
let cache_dir = directories::get_cache_directory(this);
let temp_dir = directories::get_temporary_directory(this).handle.fd();
// A URL/local tarball is re-fetched under `--force` or when its dependency
// was explicitly named on the command line, because its bytes may have
// changed at the same cache key. Verifying fresh bytes against the
// lockfile-pinned hash would reject the new content, so drop the stored
// integrity here: `ExtractTarball::run` recomputes it from the bytes and it
// is persisted back to the lockfile after extraction. Content-addressed
// resolutions (npm/git/github) keep their pinned hash.
let force_refresh_tarball = package.resolution.tag.is_tarball_cache_keyed_by_url()
&& (this.options.enable.force_install()
|| this.dependency_is_update_request(dependency_id));
let integrity = if force_refresh_tarball {
Integrity::default()
} else {
package.meta.integrity
};
// Backref address only — stored, not dereffed in this function. The tag is
// immediately popped by the next `this` use; that's fine for a stored
// back-pointer.
Expand Down Expand Up @@ -1845,7 +1890,7 @@ pub fn generate_network_task_for_tarball<'a>(
dependency_id,
skip_verify: false,
in_trusted_dependencies: this.lockfile.in_trusted_dependencies(pkg_name),
integrity: package.meta.integrity,
integrity,
url: strings::StringOrTinyString::init_append_if_needed(
url,
&mut crate::network_task::filename_store_appender(),
Expand Down
96 changes: 67 additions & 29 deletions src/install/isolated_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2194,6 +2194,10 @@ pub(crate) fn install_isolated_packages(
let uses_global_store = installer.entry_uses_global_store(entry_id);

let needs_install = installer.manager().options.enable.force_install()
// A URL/local tarball named on the command line re-fetches
// its bytes; see `force_refresh_tarball` below.
|| (pkg_res_tag.is_tarball_cache_keyed_by_url()
&& installer.manager().dependency_is_update_request(dep_id))
// A freshly-created `node_modules/.bun` only implies the
// *project-local* entries are missing; global virtual-
// store entries persist across `rm -rf node_modules` and
Expand Down Expand Up @@ -2342,41 +2346,75 @@ pub(crate) fn install_isolated_packages(
installer.manager_mut().get_cache_directory_and_abs_path();
let _ = &cache_dir_path; // dropped at scope exit

let missing_from_cache = match installer.manager().get_preinstall_state(pkg_id)
{
install::PreinstallState::Done => false,
_ => 'missing_from_cache: {
if matches!(patch_info, installer::PatchInfo::None) {
let exists = match pkg_res_tag {
ResolutionTag::Npm => {
// Reshaped for borrowck — capture length
// instead of `save()` so the path stays unborrowed.
let cache_dir_path_save = pkg_cache_dir_subpath.len();
pkg_cache_dir_subpath.append(b"package.json").assume_ok();
let exists = sys::exists_at(
let preinstall_state = installer.manager().get_preinstall_state(pkg_id);
// `--force`, or explicitly naming the dependency on the command
// line, must re-fetch a URL/local tarball: its cache folder is
// keyed by URL/path hash, so changed content hides behind the
// same key. Treat it as missing so a download/read task is
// enqueued. Gated on the state not already being `Done`: once this
// run has fetched+extracted the tarball the cache is fresh, so it
// must install from cache rather than re-enqueue. Mirrors
// `PackageInstall::package_missing_from_cache`.
let force_refresh_tarball = preinstall_state != install::PreinstallState::Done
&& pkg_res_tag.is_tarball_cache_keyed_by_url()
&& (installer.manager().options.enable.force_install()
|| installer.manager().dependency_is_update_request(dep_id))
&& {
// Skip when this run already fetched the tarball (a task
// for it is already in the queue, e.g. from the resolve
// phase after `bun update` invalidated the resolution) --
// the cache is fresh; install from it rather than
// re-enqueue into the already-drained task.
let url = if pkg_res_tag == ResolutionTag::RemoteTarball {
pkg_res.remote_tarball().slice(string_buf)
} else {
pkg_res.local_tarball().slice(string_buf)
};
!installer.manager().tarball_task_enqueued_this_run(url)
};
let missing_from_cache = if force_refresh_tarball {
true
} else {
match preinstall_state {
install::PreinstallState::Done => false,
_ => 'missing_from_cache: {
if matches!(patch_info, installer::PatchInfo::None) {
let exists = match pkg_res_tag {
ResolutionTag::Npm => {
// Reshaped for borrowck — capture length
// instead of `save()` so the path stays unborrowed.
let cache_dir_path_save = pkg_cache_dir_subpath.len();
pkg_cache_dir_subpath
.append(b"package.json")
.assume_ok();
let exists = sys::exists_at(
cache_dir,
pkg_cache_dir_subpath.slice_z(),
);
pkg_cache_dir_subpath.set_length(cache_dir_path_save);
exists
}
_ => sys::directory_exists_at(
cache_dir,
pkg_cache_dir_subpath.slice_z(),
)
.unwrap_or(false),
};
if exists {
installer.manager_mut().set_preinstall_state(
pkg_id,
install::PreinstallState::Done,
);
pkg_cache_dir_subpath.set_length(cache_dir_path_save);
exists
}
_ => sys::directory_exists_at(
cache_dir,
pkg_cache_dir_subpath.slice_z(),
)
.unwrap_or(false),
};
if exists {
installer.manager_mut().set_preinstall_state(
pkg_id,
install::PreinstallState::Done,
);
break 'missing_from_cache !exists;
}
break 'missing_from_cache !exists;
}

// TODO: why does this look like it will never work?
break 'missing_from_cache true;
// Patched packages: the non-`None` patch_info
// branch unconditionally reports missing (ported
// as-is from isolated_install.zig; its
// effectiveness is an open question upstream).
break 'missing_from_cache true;
Comment thread
robobun marked this conversation as resolved.
}
}
};

Expand Down
9 changes: 9 additions & 0 deletions src/install/resolution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,15 @@ impl Tag {
self == Tag::Git || self == Tag::Github
}

/// Tarballs whose cache folder is keyed by their URL/path hash rather than
/// by content (`cached_tarball_folder_name`). The bytes behind a URL or a
/// local path can change while the key stays the same, so `--force` must be
/// able to re-fetch and re-extract them. Npm/git/github resolutions are
/// content-addressed by version/commit and never need this.
pub fn is_tarball_cache_keyed_by_url(self) -> bool {
self == Tag::RemoteTarball || self == Tag::LocalTarball
}

pub fn can_enqueue_install_task(self) -> bool {
self == Tag::Npm
|| self == Tag::LocalTarball
Expand Down
Loading
Loading