From f0982d4f1124e66059b893fb18e75bf6bd3d7a4e Mon Sep 17 00:00:00 2001 From: Alessio Attilio Date: Thu, 22 Oct 2026 01:35:04 +0200 Subject: [PATCH] fix(resolver): refresh metadata during dedupe (#17215) pnpm dedupe resolves a fresh lockfile without an existing wanted_lockfile and with prefer_highest_version: true, so locked versions sit in preferred_versions with VersionPreference::Highest while update remains UpdateBehavior::Off. Under that combination, MirrorShortcuts::decide allowed dominant_lockfile_version and DiskFirst::decide allowed reading the on-disk metadata mirror without a conditional registry request, leaving stale packument metadata (such as deprecation messages) in place even when pnpm dedupe --check on a cold cache reported a diff. Thread refresh_metadata through ResolutionRefreshOptions into MetadataPickRequest for dedupe runs, and disable unverified on-disk mirror shortcuts when refresh_metadata is set and the network policy is not cache-first. Conditional ETag / Last-Modified headers from the mirror are still sent so unchanged packuments get a 304 response. Closes pnpm/pnpm#17208. --------- Co-authored-by: Zoltan Kochan --- .changeset/bright-pandas-refresh.md | 5 + pnpm/crates/cli/tests/suite/dedupe.rs | 82 +++++++++++++++ .../install_with_fresh_lockfile/resolution.rs | 4 + .../install_with_fresh_lockfile/resolve.rs | 2 + .../src/pick_package.rs | 6 +- .../src/pick_package/options.rs | 4 +- .../src/pick_package/route.rs | 19 +++- .../src/pick_package/route/tests.rs | 99 ++++++++++++++++--- .../resolving-resolver-base/src/resolve.rs | 9 +- 9 files changed, 202 insertions(+), 28 deletions(-) create mode 100644 .changeset/bright-pandas-refresh.md diff --git a/.changeset/bright-pandas-refresh.md b/.changeset/bright-pandas-refresh.md new file mode 100644 index 0000000000..b6fd317e6e --- /dev/null +++ b/.changeset/bright-pandas-refresh.md @@ -0,0 +1,5 @@ +--- +"pacquet": patch +--- + +`pnpm dedupe` now refreshes cached registry metadata while re-resolving locked versions [#17208](https://github.com/pnpm/pnpm/issues/17208). diff --git a/pnpm/crates/cli/tests/suite/dedupe.rs b/pnpm/crates/cli/tests/suite/dedupe.rs index 3a69846fbd..7869d3a618 100644 --- a/pnpm/crates/cli/tests/suite/dedupe.rs +++ b/pnpm/crates/cli/tests/suite/dedupe.rs @@ -6,6 +6,7 @@ use pnpm_testing_utils::{ command_env::CommandTestExt, }; use std::{fs, path::Path, process::Command}; +use walkdir::WalkDir; fn pacquet_at(workspace: &Path) -> Command { Command::cargo_bin("pnpm") @@ -14,6 +15,29 @@ fn pacquet_at(workspace: &Path) -> Command { .without_ambient_pnpm_config() } +fn add_registry_deprecation(storage: &Path, package: &str, version: &str) { + WalkDir::new(storage) + .into_iter() + .filter_map(Result::ok) + .filter(|entry| entry.file_type().is_file()) + .find_map(|entry| { + let contents = fs::read(entry.path()).ok()?; + let mut packument: serde_json::Value = serde_json::from_slice(&contents).ok()?; + if packument.get("name")?.as_str()? != package { + return None; + } + let versions = packument.get_mut("versions")?.as_object_mut()?; + let package_version = versions.get_mut(version)?.as_object_mut()?; + package_version.insert( + "deprecated".to_string(), + serde_json::json!("This package is deprecated."), + ); + fs::write(entry.path(), serde_json::to_vec(&packument).ok()?).ok()?; + Some(()) + }) + .expect("find registry package metadata"); +} + #[test] fn dedupe_preserves_auto_installed_peer_with_a_newer_major_in_another_importer() { let CommandTempCwd { root, workspace, npmrc_info, .. } = @@ -194,6 +218,64 @@ fn dedupe_writes_lockfile() { drop((root, mock_instance)); } +#[test] +fn dedupe_refreshes_cached_registry_metadata_for_locked_versions() { + let CommandTempCwd { + pacquet, + root, + workspace, + npmrc_info, + .. + } = CommandTempCwd::init().add_recording_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + mock_instance.remove_version_field("@pnpm.e2e/deprecated", "1.0.0", "deprecated"); + fs::write( + workspace.join("package.json"), + serde_json::json!({ + "dependencies": { + "@pnpm.e2e/deprecated": "1.0.0", + }, + }) + .to_string(), + ) + .expect("write package.json"); + + pacquet + .with_args(["install", "--lockfile-only"]) + .assert() + .success(); + let _ = mock_instance.take_requests(); + + let lockfile_path = workspace.join("pnpm-lock.yaml"); + let lockfile = fs::read_to_string(&lockfile_path).expect("read pnpm-lock.yaml"); + eprintln!("{lockfile}"); + assert!(!lockfile.contains("deprecated: This package is deprecated.")); + add_registry_deprecation( + &root.path().join("registry-storage"), + "@pnpm.e2e/deprecated", + "1.0.0", + ); + + pacquet_at(&workspace) + .with_args(["dedupe", "--lockfile-only"]) + .assert() + .success(); + + let dedupe_requests = mock_instance.take_requests(); + dbg!(&dedupe_requests); + assert!( + dedupe_requests + .iter() + .any(|request| { + request.if_none_match.is_some() || request.if_modified_since.is_some() + }), + ); + let deduped_lockfile = fs::read_to_string(&lockfile_path).expect("read deduped lockfile"); + eprintln!("{deduped_lockfile}"); + assert!(deduped_lockfile.contains("deprecated: This package is deprecated.")); + drop((root, mock_instance)); +} + #[test] fn dedupe_materializes_node_modules_unless_lockfile_only() { let CommandTempCwd { diff --git a/pnpm/crates/package-manager/src/install_with_fresh_lockfile/resolution.rs b/pnpm/crates/package-manager/src/install_with_fresh_lockfile/resolution.rs index 6294882969..9e600ad28b 100644 --- a/pnpm/crates/package-manager/src/install_with_fresh_lockfile/resolution.rs +++ b/pnpm/crates/package-manager/src/install_with_fresh_lockfile/resolution.rs @@ -164,6 +164,10 @@ impl<'a, Reporter: self::Reporter + 'static> ResolutionContext<'a, Reporter> { workspace_packages: self.setup.workspace_packages.clone(), update_checksums: self.install.update_checksums, update_behavior: resolver_update_behavior(&self.owned.resolution.update_seed_policy), + refresh_metadata: matches!( + self.owned.resolution.update_seed_policy, + super::UpdateSeedPolicy::KeepAllResolveAll, + ), } } diff --git a/pnpm/crates/package-manager/src/install_with_fresh_lockfile/resolve.rs b/pnpm/crates/package-manager/src/install_with_fresh_lockfile/resolve.rs index 7d3eaa4c5c..c6a8fbd95b 100644 --- a/pnpm/crates/package-manager/src/install_with_fresh_lockfile/resolve.rs +++ b/pnpm/crates/package-manager/src/install_with_fresh_lockfile/resolve.rs @@ -83,6 +83,7 @@ pub(super) struct SharedResolveOptions<'a> { /// See [`super::FreshInputs::update_checksums`]. pub update_checksums: bool, pub update_behavior: pnpm_resolving_resolver_base::UpdateBehavior, + pub refresh_metadata: bool, } impl SharedResolveOptions<'_> { @@ -108,6 +109,7 @@ impl SharedResolveOptions<'_> { refresh: pnpm_resolving_resolver_base::ResolutionRefreshOptions { update_checksums: self.update_checksums, update: self.update_behavior, + refresh_metadata: self.refresh_metadata, ..Default::default() }, policy: pnpm_resolving_resolver_base::ResolutionPolicyOptions { diff --git a/pnpm/crates/resolving-npm-resolver/src/pick_package.rs b/pnpm/crates/resolving-npm-resolver/src/pick_package.rs index 099dbad222..2485558abd 100644 --- a/pnpm/crates/resolving-npm-resolver/src/pick_package.rs +++ b/pnpm/crates/resolving-npm-resolver/src/pick_package.rs @@ -174,7 +174,11 @@ pub async fn pick_package( } // 2. Offline / pickLowestVersion / preferOffline disk read. - let disk_first = match DiskFirst::decide(ctx.cache_policy.network, opts.pick_lowest_version) { + let disk_first = match DiskFirst::decide( + ctx.cache_policy.network, + opts.pick_lowest_version, + opts.request.refresh_metadata, + ) { DiskFirst::Never => false, DiskFirst::Always => true, DiskFirst::UnlessUncacheable => state.mirror_may_answer(ctx).await, diff --git a/pnpm/crates/resolving-npm-resolver/src/pick_package/options.rs b/pnpm/crates/resolving-npm-resolver/src/pick_package/options.rs index 3a3cdc1757..8e2046f28b 100644 --- a/pnpm/crates/resolving-npm-resolver/src/pick_package/options.rs +++ b/pnpm/crates/resolving-npm-resolver/src/pick_package/options.rs @@ -119,8 +119,8 @@ pub struct MetadataPickRequest { /// the install is a pure dry-run (`--lockfile-only`, frozen /// lockfile, etc.). pub dry_run: bool, - /// `pnpm update` must see versions published since the mirror was - /// written, so it does not reuse an ETag-less mirror. + /// Revalidates the metadata mirror against the registry instead of + /// serving from unverified mirror shortcuts. pub refresh_metadata: bool, /// `true` forces this pick to use the full packument because /// the dependency carries `optionalDependencies`-specific diff --git a/pnpm/crates/resolving-npm-resolver/src/pick_package/route.rs b/pnpm/crates/resolving-npm-resolver/src/pick_package/route.rs index 7b7d007843..55b28da890 100644 --- a/pnpm/crates/resolving-npm-resolver/src/pick_package/route.rs +++ b/pnpm/crates/resolving-npm-resolver/src/pick_package/route.rs @@ -97,6 +97,9 @@ impl MirrorShortcuts { if !mirror_may_answer(network, mirror) { return None; } + if request.refresh_metadata && !network.reads_cache_first() { + return None; + } let stable_range = inputs.range_pick_is_stable(); let mirror_has_no_etag = mirror.is_some_and(|headers| headers.etag.as_deref().is_none_or(str::is_empty)); @@ -105,10 +108,7 @@ impl MirrorShortcuts { && !request.include_latest_tag && !request.update_checksums, dominant_lockfile_version: stable_range, - unvalidated_mirror: stable_range - && !request.refresh_metadata - && public_scope - && mirror_has_no_etag, + unvalidated_mirror: stable_range && public_scope && mirror_has_no_etag, }) } } @@ -135,6 +135,9 @@ impl CacheHitCheck { if network.forbids_registry() || registry_verified { return CacheHitCheck::Serve; } + if request.refresh_metadata && !network.prefers_reachable_cache() { + return CacheHitCheck::Revalidate; + } if !picked { return CacheHitCheck::Revalidate; } @@ -176,9 +179,15 @@ pub(super) enum DiskFirst { } impl DiskFirst { - pub(super) fn decide(network: NetworkPolicy, pick_lowest_version: bool) -> Self { + pub(super) fn decide( + network: NetworkPolicy, + pick_lowest_version: bool, + refresh_metadata: bool, + ) -> Self { if network.reads_cache_first() { DiskFirst::Always + } else if refresh_metadata { + DiskFirst::Never } else if pick_lowest_version { DiskFirst::UnlessUncacheable } else { diff --git a/pnpm/crates/resolving-npm-resolver/src/pick_package/route/tests.rs b/pnpm/crates/resolving-npm-resolver/src/pick_package/route/tests.rs index 2f46bb549b..3a43f5273b 100644 --- a/pnpm/crates/resolving-npm-resolver/src/pick_package/route/tests.rs +++ b/pnpm/crates/resolving-npm-resolver/src/pick_package/route/tests.rs @@ -120,7 +120,7 @@ fn a_package_excluded_from_the_release_age_keeps_the_range_shortcuts() { } #[test] -fn only_a_public_mirror_without_an_etag_skips_revalidation() { +fn an_unvalidated_mirror_skips_cache_shortcut_when_metadata_must_be_refreshed() { let untagged = headers(None, false); let empty_etag = headers(Some(""), false); let tagged = headers(Some(r#""etag""#), false); @@ -134,18 +134,15 @@ fn only_a_public_mirror_without_an_etag_skips_revalidation() { RouteInputs { public_scope: false, ..range_pick(&NO_POLICY, Some(&untagged)) }, false, ), - ( - "pnpm update", - with_request(range_pick(&NO_POLICY, Some(&untagged)), |request| { - request.refresh_metadata = true; - }), - false, - ), ]; for (name, inputs, unvalidated_mirror) in cases { let shortcuts = MirrorShortcuts::decide(&inputs).expect("the mirror may answer"); assert_eq!(shortcuts.unvalidated_mirror, unvalidated_mirror, "{name}"); } + let refresh = with_request(range_pick(&NO_POLICY, Some(&untagged)), |request| { + request.refresh_metadata = true; + }); + assert_eq!(MirrorShortcuts::decide(&refresh), None); } #[test] @@ -157,12 +154,21 @@ fn a_pinned_version_may_be_read_from_the_mirror_unless_fresh_metadata_is_needed( ("pick lowest", with_request(base, |request| request.pick_lowest_version = true), true), ("latest tag", with_request(base, |request| request.include_latest_tag = true), false), ("update checksums", with_request(base, |request| request.update_checksums = true), false), + ( + "offline refresh metadata", + with_request(RouteInputs { network: NetworkPolicy::Offline, ..base }, |request| { + request.refresh_metadata = true; + }), + true, + ), ( "tag", with_request(base, |request| request.spec_type = RegistryPackageSpecType::Tag), false, ), ]; + let refresh = with_request(base, |request| request.refresh_metadata = true); + assert_eq!(MirrorShortcuts::decide(&refresh), None); for (name, inputs, exact_version) in cases { let shortcuts = MirrorShortcuts::decide(&inputs).expect("the mirror may answer"); assert_eq!(shortcuts.exact_version, exact_version, "{name}"); @@ -228,18 +234,67 @@ fn an_unvalidated_mirror_is_fresh_within_its_max_age_either_way() { #[test] fn the_disk_is_read_first_when_the_cache_comes_first_or_the_pick_is_lowest() { + struct Case { + network: NetworkPolicy, + pick_lowest_version: bool, + refresh_metadata: bool, + disk_first: DiskFirst, + } let cases = [ - (NetworkPolicy::Online, false, DiskFirst::Never), - (NetworkPolicy::Online, true, DiskFirst::UnlessUncacheable), - (NetworkPolicy::PreferCache, false, DiskFirst::Always), - (NetworkPolicy::PreferCache, true, DiskFirst::Always), - (NetworkPolicy::Offline, false, DiskFirst::Always), + Case { + network: NetworkPolicy::Online, + pick_lowest_version: false, + refresh_metadata: false, + disk_first: DiskFirst::Never, + }, + Case { + network: NetworkPolicy::Online, + pick_lowest_version: true, + refresh_metadata: false, + disk_first: DiskFirst::UnlessUncacheable, + }, + Case { + network: NetworkPolicy::Online, + pick_lowest_version: true, + refresh_metadata: true, + disk_first: DiskFirst::Never, + }, + Case { + network: NetworkPolicy::PreferCache, + pick_lowest_version: false, + refresh_metadata: false, + disk_first: DiskFirst::Always, + }, + Case { + network: NetworkPolicy::PreferCache, + pick_lowest_version: true, + refresh_metadata: false, + disk_first: DiskFirst::Always, + }, + Case { + network: NetworkPolicy::PreferCache, + pick_lowest_version: true, + refresh_metadata: true, + disk_first: DiskFirst::Always, + }, + Case { + network: NetworkPolicy::Offline, + pick_lowest_version: false, + refresh_metadata: false, + disk_first: DiskFirst::Always, + }, ]; - for (network, pick_lowest_version, disk_first) in cases { + for Case { + network, + pick_lowest_version, + refresh_metadata, + disk_first, + } in cases + { assert_eq!( - DiskFirst::decide(network, pick_lowest_version), + DiskFirst::decide(network, pick_lowest_version, refresh_metadata), disk_first, - "network={network:?} pick_lowest_version={pick_lowest_version}", + "network={network:?} pick_lowest_version={pick_lowest_version} refresh_metadata={refresh_metadata}", ); } } @@ -299,6 +354,13 @@ fn an_unverified_cache_hit_serves_only_a_pick_a_stale_mirror_cannot_change() { CacheHitCheck::Serve, ), ("version", version_pick(&release_age, None), CacheHitCheck::Serve), + ( + "refresh metadata on a version", + with_request(version_pick(&NO_POLICY, None), |request| { + request.refresh_metadata = true; + }), + CacheHitCheck::Revalidate, + ), ("stable range", base, CacheHitCheck::StableRangePick), ( "latest tag", @@ -310,6 +372,11 @@ fn an_unverified_cache_hit_serves_only_a_pick_a_stale_mirror_cannot_change() { with_request(base, |request| request.update_checksums = true), CacheHitCheck::Revalidate, ), + ( + "refresh metadata", + with_request(base, |request| request.refresh_metadata = true), + CacheHitCheck::Revalidate, + ), ("release age", RouteInputs { policy: &release_age, ..base }, CacheHitCheck::Revalidate), ("no-downgrade", RouteInputs { policy: &no_downgrade, ..base }, CacheHitCheck::Revalidate), ( diff --git a/pnpm/crates/resolving-resolver-base/src/resolve.rs b/pnpm/crates/resolving-resolver-base/src/resolve.rs index 7a09005379..f38762885a 100644 --- a/pnpm/crates/resolving-resolver-base/src/resolve.rs +++ b/pnpm/crates/resolving-resolver-base/src/resolve.rs @@ -432,6 +432,7 @@ pub struct ResolutionRefreshOptions { /// [`Self::current_pkg`] as it is. pub repick_current_version: bool, pub update: UpdateBehavior, + pub refresh_metadata: bool, /// True only when this specific package matches the user's update /// target (e.g. `pnpm up `). Unlike `update`, this is false for /// unrelated packages that get re-resolved as a side effect of an @@ -451,12 +452,12 @@ pub struct ResolutionRefreshOptions { } impl ResolutionRefreshOptions { - /// Whether the resolution must see versions published since the - /// metadata mirror was written: an update is running, or this package - /// is its target. + /// Whether the resolution must revalidate cached metadata against the + /// registry: an update or deduplication is running, or this package is + /// its target. #[must_use] pub fn refreshes_metadata(&self) -> bool { - self.update != UpdateBehavior::Off || self.update_requested + self.update != UpdateBehavior::Off || self.update_requested || self.refresh_metadata } /// The locked runtime a runtime resolver returns as it is: a