diff --git a/.changeset/fix-minimum-release-age.md b/.changeset/fix-minimum-release-age.md new file mode 100644 index 0000000000..ad0cd04e34 --- /dev/null +++ b/.changeset/fix-minimum-release-age.md @@ -0,0 +1,6 @@ +--- +"@pnpm/resolving.npm-resolver": patch +"pnpm": patch +--- + +Fix `minimumReleaseAgeExclude` handling in npm resolution fast paths so excluded packages do not get pinned to stale versions. Excludes are honored consistently during `publishedBy` metadata selection and cache-mtime shortcuts. diff --git a/pacquet/crates/resolving-npm-resolver/src/pick_package.rs b/pacquet/crates/resolving-npm-resolver/src/pick_package.rs index 570627622f..73d4e46837 100644 --- a/pacquet/crates/resolving-npm-resolver/src/pick_package.rs +++ b/pacquet/crates/resolving-npm-resolver/src/pick_package.rs @@ -60,7 +60,7 @@ use chrono::{DateTime, Utc}; use dashmap::DashMap; use derive_more::{Display, Error}; use miette::Diagnostic; -use pacquet_config::version_policy::PackageVersionPolicy; +use pacquet_config::version_policy::{PackageVersionPolicy, PolicyMatch}; use pacquet_network::{AuthHeaders, ThrottledClient}; use pacquet_registry::{Package, PackageVersion}; use pacquet_resolving_resolver_base::VersionSelectors; @@ -563,7 +563,15 @@ pub async fn pick_package( } // 4. publishedBy mtime shortcut. + // + // Fully excluded packages (`minimumReleaseAgeExclude: ['pkg']`) treat + // minimumReleaseAge as disabled, so this shortcut must not bypass + // revalidation against potentially stale on-disk metadata. if let Some(published_by) = opts.published_by + && !matches!( + opts.published_by_exclude.map(|policy| policy.matches(&spec.name)), + Some(PolicyMatch::AnyVersion), + ) && let Some(mtime) = pkg_mirror.as_deref().and_then(get_file_mtime) && mtime >= published_by { @@ -1049,11 +1057,10 @@ async fn maybe_upgrade_abbreviated_meta_for_release_age if meta.time.is_some() { return Ok(UpgradeOutcome { meta, upgraded: false }); } - if let Some(policy) = opts.published_by_exclude { - use pacquet_config::version_policy::PolicyMatch; - if matches!(policy.matches(&spec.name), PolicyMatch::AnyVersion) { - return Ok(UpgradeOutcome { meta, upgraded: false }); - } + if let Some(policy) = opts.published_by_exclude + && matches!(policy.matches(&spec.name), PolicyMatch::AnyVersion) + { + return Ok(UpgradeOutcome { meta, upgraded: false }); } // Inclusive `<=` at the boundary: matches the per-version // `<=` filter in `filter_pkg_metadata_by_publish_date`. When diff --git a/pacquet/crates/resolving-npm-resolver/src/pick_package/tests.rs b/pacquet/crates/resolving-npm-resolver/src/pick_package/tests.rs index e288c9bf90..1686675394 100644 --- a/pacquet/crates/resolving-npm-resolver/src/pick_package/tests.rs +++ b/pacquet/crates/resolving-npm-resolver/src/pick_package/tests.rs @@ -3,6 +3,7 @@ use pretty_assertions::assert_eq; use tempfile::TempDir; use chrono::{DateTime, Utc}; +use pacquet_config::version_policy::create_package_version_policy; use super::{ InMemoryPackageMetaCache, PackageMetaCache, PickPackageContext, PickPackageError, @@ -837,6 +838,142 @@ async fn published_by_skips_upgrade_when_modified_equals_cutoff() { abbrev_mock.assert_async().await; } +/// Excluded packages must skip abbreviated->full upgrade even when +/// `modified` is newer than the cutoff, because minimumReleaseAge is +/// disabled for `PolicyMatch::AnyVersion`. +#[tokio::test] +async fn published_by_exclude_skips_upgrade_for_abbreviated_meta_without_time() { + let mut server = mockito::Server::new_async().await; + let abbrev_mock = server + .mock("GET", "/acme") + .match_header( + "accept", + "application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*", + ) + .with_status(200) + .with_body(ABBREVIATED_BODY) + .expect(1) + .create_async() + .await; + + // No full-metadata mock on purpose: excluded packages must not trigger + // the upgrade fetch even when abbreviated metadata has no `time` field. + let cache_dir = TempDir::new().expect("tempdir"); + let registry = format!("{}/", server.url()); + let http_client = ThrottledClient::default(); + let auth_headers = AuthHeaders::default(); + let meta_cache = InMemoryPackageMetaCache::default(); + let fetch_locker = shared_packument_fetch_locker(); + let ctx = PickPackageContext { + http_client: &http_client, + auth_headers: &auth_headers, + meta_cache: &meta_cache, + fetch_locker: &fetch_locker, + cache_dir: Some(cache_dir.path()), + offline: false, + prefer_offline: false, + ignore_missing_time_field: false, + full_metadata: false, + }; + + let policy = create_package_version_policy(["acme"]).expect("policy"); + let mut opts = default_opts(®istry); + opts.published_by = Some(parse_cutoff("2020-01-01T00:00:00Z")); + opts.published_by_exclude = Some(&policy); + + let result = pick_package(&ctx, &range_spec("acme", "^1.0.0"), &opts).await.expect("ok"); + assert_eq!( + result.picked_package.expect("picked").version.to_string(), + "1.0.0", + "exclude policy should bypass release-age upgrade and pick from abbreviated meta", + ); + abbrev_mock.assert_async().await; +} + +/// Fully excluded packages (`minimumReleaseAgeExclude: ['acme']`) must bypass +/// the publishedBy file-mtime cache shortcut, otherwise a stale abbreviated +/// mirror can pin resolution to an old latest forever until the cutoff window +/// moves past the file mtime. +#[tokio::test] +async fn published_by_excluded_package_bypasses_mtime_shortcut_and_revalidates() { + let mut server = mockito::Server::new_async().await; + // Fresh network metadata has 1.1.0 as latest. + let network_mock = server + .mock("GET", "/acme") + .with_status(200) + .with_header("etag", r#"W/"fresh""#) + .with_body(PACKAGE_BODY) + .expect(1) + .create_async() + .await; + + let cache_dir = TempDir::new().expect("tempdir"); + let registry = format!("{}/", server.url()); + + // Stale abbreviated mirror missing 1.1.0 entirely. + let stale_body = r#"{ + "name": "acme", + "dist-tags": { "latest": "1.0.0" }, + "modified": "2024-01-01T00:00:00.000Z", + "versions": { + "1.0.0": { + "name": "acme", + "version": "1.0.0", + "dist": { + "integrity": "sha512-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA==", + "shasum": "0000000000000000000000000000000000000000", + "tarball": "https://registry/acme-1.0.0.tgz" + } + } + } + }"#; + let preloaded: pacquet_registry::Package = + serde_json::from_str(stale_body).expect("parse stale packument"); + persist_meta_to_mirror(cache_dir.path(), ABBREVIATED_META_DIR, ®istry, &preloaded) + .expect("warm stale mirror"); + let mirror_path = + get_pkg_mirror_path(cache_dir.path(), ABBREVIATED_META_DIR, ®istry, "acme") + .expect("path"); + let forced_mtime: std::time::SystemTime = parse_cutoff("2024-01-01T00:00:00Z").into(); + std::fs::OpenOptions::new() + .write(true) + .open(&mirror_path) + .expect("open stale mirror") + .set_times(std::fs::FileTimes::new().set_modified(forced_mtime)) + .expect("set stale mirror mtime"); + + let http_client = ThrottledClient::default(); + let auth_headers = AuthHeaders::default(); + let meta_cache = InMemoryPackageMetaCache::default(); + let fetch_locker = shared_packument_fetch_locker(); + let ctx = PickPackageContext { + http_client: &http_client, + auth_headers: &auth_headers, + meta_cache: &meta_cache, + fetch_locker: &fetch_locker, + cache_dir: Some(cache_dir.path()), + offline: false, + prefer_offline: false, + ignore_missing_time_field: false, + full_metadata: false, + }; + + let policy = create_package_version_policy(["acme"]).expect("policy"); + let mut opts = default_opts(®istry); + // Keep the mtime-guard condition deterministic: mirror mtime is set + // explicitly to 2024-01-01 above. + opts.published_by = Some(parse_cutoff("2020-01-01T00:00:00Z")); + opts.published_by_exclude = Some(&policy); + + let result = pick_package(&ctx, &range_spec("acme", "^1.0.0"), &opts).await.expect("ok"); + assert_eq!( + result.picked_package.expect("picked").version.to_string(), + "1.1.0", + "excluded package should revalidate stale mirror and pick fresh latest", + ); + network_mock.assert_async().await; +} + /// Concurrent `pick_package` calls for the same `(registry, name)` /// coalesce into a single network fetch. Mirrors pnpm's /// [`runLimited(pkgMirror, …)`](https://github.com/pnpm/pnpm/blob/f657b5cb44/resolving/npm-resolver/src/pickPackage.ts#L52-L64) diff --git a/resolving/npm-resolver/src/pickPackage.ts b/resolving/npm-resolver/src/pickPackage.ts index c7fa712084..b641320e84 100644 --- a/resolving/npm-resolver/src/pickPackage.ts +++ b/resolving/npm-resolver/src/pickPackage.ts @@ -294,7 +294,7 @@ export async function pickPackage ( } } } - if (opts.publishedBy) { + if (opts.publishedBy && opts.publishedByExclude?.(spec.name) !== true) { const mtime = await limit(async () => getFileMtime(pkgMirror)) if (mtime != null && mtime >= opts.publishedBy) { metaCachedInStore = metaCachedInStore ?? await limit(async () => loadMeta(pkgMirror)) diff --git a/resolving/npm-resolver/test/publishedBy.test.ts b/resolving/npm-resolver/test/publishedBy.test.ts index 4d8b75418b..ed176cc42d 100644 --- a/resolving/npm-resolver/test/publishedBy.test.ts +++ b/resolving/npm-resolver/test/publishedBy.test.ts @@ -8,6 +8,7 @@ import { createNpmResolver } from '@pnpm/resolving.npm-resolver' import { fixtures } from '@pnpm/test-fixtures' import type { Registries } from '@pnpm/types' import { loadJsonFileSync } from 'load-json-file' +import semver from 'semver' import { temporaryDirectory } from 'tempy' import { getMockAgent, retryLoadJsonFile, setupMockAgent, teardownMockAgent } from './utils/index.js' @@ -516,7 +517,7 @@ test('use cached metadata based on file mtime when publishedBy is set', async () const cacheDir2 = path.join(cacheDir, `${ABBREVIATED_META_DIR}/registry.npmjs.org`) fs.mkdirSync(cacheDir2, { recursive: true }) const cachePath = path.join(cacheDir2, 'is-positive.jsonl') - const headers = JSON.stringify({ modified: isPositiveAbbreviatedMeta.modified }) + const headers = JSON.stringify({ etag: '"mtime-shortcut-test"', modified: isPositiveAbbreviatedMeta.modified }) fs.writeFileSync(cachePath, `${headers}\n${JSON.stringify(isPositiveAbbreviatedMeta)}`, 'utf8') // No mock agent intercepts — the test verifies no network request is made. @@ -535,3 +536,52 @@ test('use cached metadata based on file mtime when publishedBy is set', async () expect(resolveResult!.resolvedVia).toBe('npm-registry') expect(resolveResult!.id).toBe('is-positive@3.1.0') }) + +test('excluded packages bypass the mtime cache shortcut and refresh stale metadata', async () => { + const cacheDir = temporaryDirectory() + const abbrevCacheDir = path.join(cacheDir, `${ABBREVIATED_META_DIR}/registry.npmjs.org`) + fs.mkdirSync(abbrevCacheDir, { recursive: true }) + const cachePath = path.join(abbrevCacheDir, 'is-positive.jsonl') + + // Stale abbreviated cache: every version newer than 3.0.0 is missing. + // If the publishedBy mtime shortcut is taken, resolution incorrectly + // freezes on 3.0.0 forever for recently-written cache files. + const staleVersions = Object.fromEntries( + Object.entries(isPositiveAbbreviatedMeta.versions) + .filter(([version]) => !semver.gt(version, '3.0.0')) + ) + const staleCachedMeta = { + ...isPositiveAbbreviatedMeta, + versions: staleVersions, + 'dist-tags': { + ...isPositiveAbbreviatedMeta['dist-tags'], + latest: '3.0.0', + }, + } + const headers = JSON.stringify({ etag: '"exclude-refresh-test"', modified: staleCachedMeta.modified }) + fs.writeFileSync(cachePath, `${headers}\n${JSON.stringify(staleCachedMeta)}`, 'utf8') + const forcedMtime = new Date('2024-01-01T00:00:00.000Z') + fs.utimesSync(cachePath, forcedMtime, forcedMtime) + + // Network has fresh metadata with 3.1.0 as latest. + getMockAgent().get(registries.default.replace(/\/$/, '')) + .intercept({ path: '/is-positive', method: 'GET' }) + .reply(200, isPositiveAbbreviatedMeta) + + const { resolveFromNpm } = createResolveFromNpm({ + storeDir: temporaryDirectory(), + cacheDir, + registries, + }) + const resolveResult = await resolveFromNpm({ alias: 'is-positive', bareSpecifier: '^3.0.0' }, { + // Mirror mtime is explicitly set after this cutoff to guarantee the + // mtime shortcut condition is satisfied in all environments. + publishedBy: new Date('2020-01-01T00:00:00.000Z'), + // But excluded packages should behave like publishedBy is off and + // still revalidate stale metadata against the registry. + publishedByExclude: (pkgName) => pkgName === 'is-positive', + }) + + expect(resolveResult!.resolvedVia).toBe('npm-registry') + expect(resolveResult!.id).toBe('is-positive@3.1.0') +})