From 9b0a460c8d5ea6db94919ccfd7728b37d9019198 Mon Sep 17 00:00:00 2001 From: Brandon Cheng Date: Tue, 7 Apr 2026 17:46:27 -0400 Subject: [PATCH] fix: non-deterministic resolution causing `pnpm dedupe --check` to unexpectedly fail (#11110) * test: ensure prerelease weighting is correct * fix: use higher weight for package versions already in lockfile * test: remove fundamentally incompatible test * fix(test): use undici MockAgent instead of nock for HTTP mocking nock only patches Node's built-in http/https modules, but pnpm uses undici for HTTP requests. Replace nock with @pnpm/testing.mock-agent (which wraps undici's MockAgent) so the regression test actually intercepts registry metadata requests. * fix(benchmarks): show errors from store populate step The populate step redirected both stdout and stderr to /dev/null, hiding the actual error when pnpm install fails during benchmarks. * fix(benchmarks): replace deprecated packages in benchmark fixture The old fixture used deprecated babel 6, gulp, and other legacy packages whose transitive dependencies (e.g. es-abstract) are missing the "time" field in registry metadata, causing ERR_PNPM_MISSING_TIME with time-based resolution mode. Replace with modern equivalents (babel 7, webpack 5, MUI, Redux Toolkit, etc.) that maintain a similar dependency tree size (~1300 packages) while using well-maintained packages with proper registry metadata. * fix(benchmarks): drop eslint plugins that pull in es-abstract eslint-plugin-react, eslint-plugin-import, and eslint-plugin-jsx-a11y transitively depend on es-abstract, whose registry metadata lacks the "time" field. Replace them with eslint-plugin-prettier to avoid ERR_PNPM_MISSING_TIME with time-based resolution. --------- Co-authored-by: Zoltan Kochan --- .changeset/calm-ducks-post.md | 12 ++ benchmarks/bench.sh | 2 +- benchmarks/fixture.package.json | 156 ++++++++---------- installing/deps-installer/package.json | 1 + .../deps-installer/test/install/dedupe.ts | 16 -- .../test/install/prereleaseWeights.ts | 128 ++++++++++++++ installing/deps-installer/tsconfig.json | 3 + lockfile/preferred-versions/src/index.ts | 57 ++++++- pnpm-lock.yaml | 3 + resolving/resolver-base/src/index.ts | 10 ++ 10 files changed, 277 insertions(+), 111 deletions(-) create mode 100644 .changeset/calm-ducks-post.md create mode 100644 installing/deps-installer/test/install/prereleaseWeights.ts diff --git a/.changeset/calm-ducks-post.md b/.changeset/calm-ducks-post.md new file mode 100644 index 0000000000..9439ff5044 --- /dev/null +++ b/.changeset/calm-ducks-post.md @@ -0,0 +1,12 @@ +--- +"@pnpm/lockfile.preferred-versions": patch +"@pnpm/installing.deps-installer": patch +"@pnpm/resolving.resolver-base": patch +pnpm: patch +--- + +Fixed a resolution bug that could cause `pnpm dedupe --check` to fail unexpectedly. + +When adding new dependencies to `package.json`, pnpm generally reuses existing versions in the `pnpm-lock.yaml` if they are satisfied by the version range specifier. There was an edge case where pnpm would instead resolve to a newly released version of a dependency. This is particularly problematic for `pnpm dedupe --check`, since a new version of a dependency published to the NPM registry could cause this check to suddenly fail. For details of this bug, see [#10626](https://github.com/pnpm/pnpm/issues/10626). This bug has been fixed. + +The fix necessitated a behavioral change: In some cases, pnpm was previously able to automatically dedupe a newly used dependency deep in the dependency graph without needing to run `pnpm dedupe`. This behavior was supported by the non-determinism that is now corrected. We believe fixing this non-determinism is more important than preserving an automatic dedupe heuristic that didn't handle all cases. The `pnpm dedupe` command can still be used to clean up dependencies that aren't automatically deduped on `pnpm install`. diff --git a/benchmarks/bench.sh b/benchmarks/bench.sh index 82498ff87f..c71d83faa8 100755 --- a/benchmarks/bench.sh +++ b/benchmarks/bench.sh @@ -115,7 +115,7 @@ for i in "${!VARIANTS[@]}"; do bin="${VARIANT_BINS[$i]}" echo "Populating store and cache for $label..." rm -rf "$dir/node_modules" "$dir/pnpm-lock.yaml" - cd "$dir" && node "$bin" install --ignore-scripts --no-frozen-lockfile >/dev/null 2>&1 + cd "$dir" && node "$bin" install --ignore-scripts --no-frozen-lockfile if [ ! -f "$dir/pnpm-lock.yaml" ]; then echo "error: pnpm-lock.yaml was not created for $label in $dir" >&2 exit 1 diff --git a/benchmarks/fixture.package.json b/benchmarks/fixture.package.json index 7cd040a5f3..0fe61ad185 100644 --- a/benchmarks/fixture.package.json +++ b/benchmarks/fixture.package.json @@ -2,117 +2,95 @@ "name": "bench-project", "version": "1.0.0", "dependencies": { - "animate.less": "^2.2.0", + "@babel/core": "^7.24.0", + "@babel/preset-env": "^7.24.0", + "@babel/preset-react": "^7.23.3", + "@babel/preset-typescript": "^7.23.3", + "@babel/plugin-transform-runtime": "^7.24.0", + "@babel/runtime": "^7.24.0", + "@emotion/react": "^11.11.3", + "@emotion/styled": "^11.11.0", + "@mui/material": "^5.15.7", + "@reduxjs/toolkit": "^2.1.0", + "@tanstack/react-query": "^5.18.1", + "@tanstack/react-table": "^8.11.6", + "@types/node": "^20.11.10", + "@types/react": "^18.2.48", + "@types/react-dom": "^18.2.18", + "@typescript-eslint/eslint-plugin": "^7.0.1", + "@typescript-eslint/parser": "^7.0.1", "autoprefixer": "^10.4.17", - "babel-core": "^6.26.3", - "babel-eslint": "^10.1.0", - "babel-loader": "^9.1.3", - "babel-plugin-lodash": "^3.3.4", - "babel-plugin-module-resolver": "^5.0.0", - "babel-plugin-transform-decorators-legacy": "^1.3.5", - "babel-plugin-transform-runtime": "^6.23.0", - "babel-polyfill": "^6.26.0", - "babel-preset-es2015": "^6.24.1", - "babel-preset-react": "^6.24.1", - "babel-preset-react-hmre": "^1.1.1", - "babel-preset-stage-1": "^6.24.1", - "babel-runtime": "^6.26.0", + "axios": "^1.6.7", + "chart.js": "^4.4.1", "clean-webpack-plugin": "^4.0.0", - "core-decorators": "^0.20.0", - "css-loader": "^6.9.0", - "css-mqpacker": "^7.0.0", + "css-loader": "^6.9.1", "cssnano": "^6.0.3", - "custom-event-polyfill": "^1.0.7", - "draft-js": "^0.11.7", - "ejs": "^3.1.9", - "eslint": "^8.56.0", - "eslint-config-airbnb": "^19.0.4", - "eslint-import-resolver-webpack": "^0.13.8", - "eslint-plugin-import": "^2.29.1", - "eslint-plugin-jsx-a11y": "^6.8.0", - "eslint-plugin-react": "^7.33.2", + "date-fns": "^3.3.1", + "eslint": "^9.9.0", + "eslint-config-prettier": "^9.1.0", + "eslint-plugin-prettier": "^5.1.3", "express": "^4.18.2", - "express-http-proxy": "^2.0.0", - "font-awesome": "^4.7.0", - "fready": "^1.0.0", - "glob": "^10.3.10", - "gulp": "^4.0.2", - "gulp-concat": "^2.6.1", - "gulp-csslint": "^1.0.1", - "gulp-cssnano": "^2.1.3", - "gulp-eol": "^0.2.0", - "gulp-less": "^5.0.0", - "gulp-livereload": "^4.0.2", - "gulp-minify-css": "^1.2.4", - "gulp-postcss": "^9.1.0", - "gulp-rename": "^2.0.0", - "gulp-util": "^3.0.8", - "happypack": "^5.0.1", - "highcharts": "^11.3.0", - "highcharts-solid-gauge": "^0.1.7", - "history": "^5.3.0", - "howler": "^2.2.4", - "imports-loader": "^5.0.0", + "formik": "^2.4.5", + "framer-motion": "^11.0.3", + "html-webpack-plugin": "^5.6.0", + "husky": "^9.0.7", + "i18next": "^23.8.2", + "immer": "^10.0.3", "jquery": "^3.7.1", - "jquery-ui": "1.13.2", "js-cookie": "^3.0.5", - "json-loader": "^0.5.7", - "leftpad": "^0.0.1", "less": "^4.2.0", - "lesshat": "^4.1.0", + "less-loader": "^12.2.0", + "lint-staged": "^15.2.1", "lodash": "^4.17.21", - "medium-draft": "^0.5.18", + "mini-css-extract-plugin": "^2.7.7", "mobx": "^6.12.0", - "mobx-react": "^9.1.0", + "mobx-react-lite": "^4.0.5", "moment": "^2.30.1", - "moment-range": "^4.0.2", "moment-timezone": "^0.5.44", - "password-policy": "0.0.3", - "postcss-reporter": "^7.1.0", - "progress": "^2.0.3", + "postcss": "^8.4.33", + "postcss-loader": "^8.1.0", + "postcss-preset-env": "^9.3.0", + "prettier": "^3.2.4", "qs": "^6.11.2", - "raw-loader": "^4.0.2", - "rc-slider": "^10.5.0", "react": "^18.2.0", - "react-addons-css-transition-group": "^15.6.2", - "react-addons-shallow-compare": "^15.6.3", "react-dnd": "^16.0.1", "react-dnd-html5-backend": "^16.0.1", "react-dom": "^18.2.0", - "react-draft-wysiwyg": "^1.15.0", "react-dropzone": "^14.2.3", - "react-grid-layout": "^1.4.4", - "react-highcharts": "^16.1.0", - "react-hot-loader": "4.13.1", - "react-input-calendar": "^0.5.4", - "react-lazyload": "^3.2.0", - "react-measure": "^2.5.2", - "react-mixin": "^5.0.0", - "react-responsive": "9.0.2", - "react-responsive-tabs": "^4.4.3", - "react-router": "^6.21.2", - "react-router-dom": "^6.21.2", - "react-select-plus": "1.0.0-rc.3.patch12", - "react-skylight": "^0.5.1", - "react-sortablejs": "^6.1.4", - "react-tappable": "^1.0.4", - "react-tooltip": "5.25.2", + "react-helmet-async": "^2.0.4", + "react-hook-form": "^7.49.3", + "react-i18next": "^14.0.5", + "react-redux": "^9.1.0", + "react-router": "^6.21.3", + "react-router-dom": "^6.21.3", + "react-select": "^5.8.0", + "react-tooltip": "^5.25.2", "react-virtualized": "^9.22.5", - "react-waypoint": "^10.3.0", + "recharts": "^2.12.0", + "redux": "^5.0.1", + "sass": "^1.70.0", + "sass-loader": "^14.1.0", "sortablejs": "^1.15.2", "style-loader": "^3.3.4", - "stylelint": "^16.1.0", - "superagent": "^8.1.2", - "uglify-js": "^3.17.4", + "stylelint": "^16.2.1", + "stylelint-config-standard": "^36.0.0", + "terser-webpack-plugin": "^5.3.10", + "typescript": "^5.3.3", "uuid": "^9.0.1", - "verge": "^1.10.2", + "webpack": "^5.90.1", "webpack-bundle-analyzer": "^4.10.1", - "webpack-hot-middleware": "^2.26.0", - "webpack-notifier": "^1.15.0", - "webpack-split-by-path": "^2.0.0", - "whatwg-fetch": "^3.6.20" + "webpack-cli": "^5.1.4", + "webpack-dev-server": "^4.15.2", + "webpack-merge": "^5.10.0", + "whatwg-fetch": "^3.6.20", + "yup": "^1.3.3", + "zod": "^3.22.4", + "zustand": "^4.5.0" }, "devDependencies": { - "nan-as": "^1.6.1" + "@testing-library/jest-dom": "^6.4.1", + "@testing-library/react": "^14.2.1", + "jest": "^29.7.0", + "jest-environment-jsdom": "^29.7.0" } -} \ No newline at end of file +} diff --git a/installing/deps-installer/package.json b/installing/deps-installer/package.json index 271e36f802..c5fcb05e6a 100644 --- a/installing/deps-installer/package.json +++ b/installing/deps-installer/package.json @@ -134,6 +134,7 @@ "@pnpm/pkg-manifest.reader": "workspace:*", "@pnpm/prepare": "workspace:*", "@pnpm/registry-mock": "catalog:", + "@pnpm/resolving.registry.types": "workspace:*", "@pnpm/store.cafs": "workspace:*", "@pnpm/store.index": "workspace:*", "@pnpm/store.path": "workspace:*", diff --git a/installing/deps-installer/test/install/dedupe.ts b/installing/deps-installer/test/install/dedupe.ts index 39e6b6ced4..ae96db070e 100644 --- a/installing/deps-installer/test/install/dedupe.ts +++ b/installing/deps-installer/test/install/dedupe.ts @@ -177,22 +177,6 @@ test('prefer version of package that also satisfies the range of the same packag ) }) -test('dedupe subdependency when a newer version of the same package is installed', async () => { - const project = prepareEmpty() - - await addDistTag({ package: '@pnpm.e2e/dep-of-pkg-with-1-dep', version: '100.0.0', distTag: 'latest' }) - - const { updatedManifest: manifest } = await addDependenciesToPackage({}, ['@pnpm.e2e/dep-of-pkg-with-1-dep@100.0.0', '@pnpm.e2e/pkg-with-1-dep@100.0.0'], testDefaults()) - - await addDistTag({ package: '@pnpm.e2e/dep-of-pkg-with-1-dep', version: '100.1.0', distTag: 'latest' }) - - await addDependenciesToPackage(manifest, ['@pnpm.e2e/dep-of-pkg-with-1-dep@100.1.0'], testDefaults()) - - const lockfile = project.readLockfile() - expect(lockfile.packages).toHaveProperty(['@pnpm.e2e/dep-of-pkg-with-1-dep@100.1.0']) - expect(lockfile.packages).not.toHaveProperty(['@pnpm.e2e/dep-of-pkg-with-1-dep@100.0.0']) -}) - test('when resolving dependencies, prefer versions that are used by direct dependencies over versions used in subdeps', async () => { await addDistTag({ package: '@pnpm.e2e/foo', version: '100.1.0', distTag: 'latest' }) const project = prepareEmpty() diff --git a/installing/deps-installer/test/install/prereleaseWeights.ts b/installing/deps-installer/test/install/prereleaseWeights.ts new file mode 100644 index 0000000000..a82463a4a0 --- /dev/null +++ b/installing/deps-installer/test/install/prereleaseWeights.ts @@ -0,0 +1,128 @@ +import path from 'node:path' + +import { type MutatedProject, mutateModules, type MutateModulesOptions, type ProjectOptions } from '@pnpm/installing.deps-installer' +import { prepareEmpty } from '@pnpm/prepare' +import type { PackageMeta } from '@pnpm/resolving.registry.types' +import { getMockAgent, setupMockAgent, teardownMockAgent } from '@pnpm/testing.mock-agent' +import type { ProjectId, ProjectManifest, ProjectRootDir } from '@pnpm/types' + +import { testDefaults } from '../utils/index.js' + +afterEach(async () => { + await teardownMockAgent() +}) + +// Regression test for https://github.com/pnpm/pnpm/issues/10626 +test('prerelease specifiers do not cause not-yet-used version to be resolved', async () => { + const rootProject = prepareEmpty() + const lockfileDir = rootProject.dir() + + const name = '@pnpm.e2e/prerelease' + + const projects: Record = { + ['a' as ProjectId]: { + name: 'a', + dependencies: { + [name]: '^1.1.0-beta', + }, + }, + ['b' as ProjectId]: { + name: 'b', + dependencies: { + [name]: '^1.2.0-beta', + }, + }, + ['c' as ProjectId]: { + name: 'c', + }, + } + const allProjects: ProjectOptions[] = Object.entries(projects) + .map(([id, manifest]) => ({ + buildIndex: 0, + manifest, + rootDir: path.resolve(id) as ProjectRootDir, + })) + const options = { + ...testDefaults( + { allProjects }, + { retry: { retries: 0 } } + ), + lockfileDir, + lockfileOnly: true, + resolutionMode: 'highest', + } satisfies MutateModulesOptions + + const installProjects: MutatedProject[] = Object.entries(projects) + .map(([id, manifest]) => ({ + mutation: 'install', + id, + manifest, + rootDir: path.resolve(id) as ProjectRootDir, + })) + + const meta: PackageMeta = { + name, + versions: { + '1.1.0-beta': { + name, + version: '1.1.0-beta', + // Generated locally through: echo '1.1.0-beta' | sha1sum + dist: { shasum: '7957736c00bc1e5a875e5e4f8f48d8f5a3830866', tarball: `${options.registries.default}/${name}-1.1.0-beta.tgz` }, + }, + '1.2.0-beta': { + name, + version: '1.2.0-beta', + dist: { shasum: '50c0586b05b59205f39610d63cc38ea04954182c', tarball: `${options.registries.default}/${name}-1.2.0-beta.tgz` }, + }, + }, + 'dist-tags': { + latest: '1.2.0-beta', + }, + } + + await setupMockAgent() + const registryUrl = options.registries.default.replace(/\/$/, '') + // cspell:disable-next-line + const metadataPath = '/@pnpm.e2e%2Fprerelease' + + getMockAgent().get(registryUrl) + .intercept({ path: metadataPath, method: 'GET' }) + .reply(200, meta) + + await mutateModules(installProjects, options) + + { + const lockfile = rootProject.readLockfile() + expect(lockfile.importers['a' as ProjectId].dependencies?.[name]).toEqual({ + specifier: '^1.1.0-beta', + version: '1.1.0-beta', + }) + expect(lockfile.importers['b' as ProjectId].dependencies?.[name]).toEqual({ + specifier: '^1.2.0-beta', + version: '1.2.0-beta', + }) + } + + // Simulate publishing a new 1.2.0 version. + meta.versions['1.2.0'] = { + name, + version: '1.2.0', + dist: { shasum: 'f95c23882c82328c872ac94af630c49ae57f37bb', tarball: `${options.registries.default}/${name}-1.2.0.tgz` }, + } + meta['dist-tags'].latest = '1.2.0' + + options.storeController.clearResolutionCache() + + getMockAgent().get(registryUrl) + .intercept({ path: metadataPath, method: 'GET' }) + .reply(200, meta) + + projects['c' as ProjectId].dependencies = { [name]: '^1.2.0-beta' } + await mutateModules(installProjects, options) + + const lockfile = rootProject.readLockfile() + expect(lockfile.importers['c' as ProjectId].dependencies?.[name]).toEqual({ + specifier: '^1.2.0-beta', + version: '1.2.0-beta', + }) +}) diff --git a/installing/deps-installer/tsconfig.json b/installing/deps-installer/tsconfig.json index 47533ea976..c85692086a 100644 --- a/installing/deps-installer/tsconfig.json +++ b/installing/deps-installer/tsconfig.json @@ -147,6 +147,9 @@ { "path": "../../resolving/parse-wanted-dependency" }, + { + "path": "../../resolving/registry/types" + }, { "path": "../../resolving/resolver-base" }, diff --git a/lockfile/preferred-versions/src/index.ts b/lockfile/preferred-versions/src/index.ts index 71293ae569..7470031701 100644 --- a/lockfile/preferred-versions/src/index.ts +++ b/lockfile/preferred-versions/src/index.ts @@ -1,6 +1,12 @@ import { nameVerFromPkgSnapshot, type PackageSnapshots } from '@pnpm/lockfile.utils' import { getAllDependenciesFromManifest } from '@pnpm/pkg-manifest.utils' -import { DIRECT_DEP_SELECTOR_WEIGHT, type PreferredVersions } from '@pnpm/resolving.resolver-base' +import { + DIRECT_DEP_SELECTOR_WEIGHT, + EXISTING_VERSION_SELECTOR_WEIGHT, + type PreferredVersions, + type VersionSelectorType, + type VersionSelectorWithWeight, +} from '@pnpm/resolving.resolver-base' import type { DependencyManifest, ProjectManifest } from '@pnpm/types' import getVersionSelectorType from 'version-selector-type' @@ -27,12 +33,53 @@ export function getPreferredVersionsFromLockfileAndManifests ( } function addPreferredVersionsFromLockfile (snapshots: PackageSnapshots, preferredVersions: PreferredVersions): void { + // The snapshots object can contain multiple entries with the same package + // name and version. This is because a dependency can appear multiple times + // with the same version in the lockfile due to peer dependency resolution. To + // avoid inflating the weight of package versions that appear multiple times, + // generate a map with only the unique set to iterate over. + const uniqueNameVersions: Record> = {} for (const [depPath, snapshot] of Object.entries(snapshots)) { const { name, version } = nameVerFromPkgSnapshot(depPath, snapshot) - if (!preferredVersions[name]) { - preferredVersions[name] = { [version]: 'version' } - } else if (!preferredVersions[name][version]) { - preferredVersions[name][version] = 'version' + uniqueNameVersions[name] ??= new Set() + uniqueNameVersions[name].add(version) + } + + for (const [name, versions] of Object.entries(uniqueNameVersions)) { + for (const version of versions) { + preferredVersions[name] ??= {} + + const existingSelector = preferredVersions[name][version] + if (existingSelector == null) { + preferredVersions[name][version] = { selectorType: 'version', weight: EXISTING_VERSION_SELECTOR_WEIGHT } + continue + } + + // The lookup for this selector was for an exact version and not a range + // or tag. If there's an existing selector and it's not for a version, + // that's unexpected and our program state is corrupted. + const existingSelectorType = typeof existingSelector === 'string' + ? existingSelector + : existingSelector.selectorType + if (existingSelectorType !== 'version') { + throw new Error(`Encountered unexpected version selector '${existingSelectorType}' for dependency '${name}@${version}'`) + } + + // There might be an existing selector on this exact version from a direct + // dependency. If so, we should increase its weight. This allows a version + // present in the lockfile that's also used by a direct dependency to be + // considered at a higher priority than a package with only one of the two + // criteria. + preferredVersions[name][version] = addWeightToVersionSelector(existingSelector, EXISTING_VERSION_SELECTOR_WEIGHT) } } } + +function addWeightToVersionSelector ( + selector: VersionSelectorWithWeight | VersionSelectorType, + weight: number +): VersionSelectorWithWeight { + return typeof selector === 'string' + ? { selectorType: selector, weight: weight + 1 } + : { selectorType: selector.selectorType, weight: selector.weight + weight } +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ba8836a2a5..dbc09c1227 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -5398,6 +5398,9 @@ importers: '@pnpm/registry-mock': specifier: 'catalog:' version: 5.2.4(verdaccio@6.3.2(encoding@0.1.13)(typanion@3.14.0)) + '@pnpm/resolving.registry.types': + specifier: workspace:* + version: link:../../resolving/registry/types '@pnpm/store.cafs': specifier: workspace:* version: link:../../store/cafs diff --git a/resolving/resolver-base/src/index.ts b/resolving/resolver-base/src/index.ts index f297a51d6b..9a27497873 100644 --- a/resolving/resolver-base/src/index.ts +++ b/resolving/resolver-base/src/index.ts @@ -97,6 +97,16 @@ export type WorkspacePackages = Map // It is important to give a bigger weight to direct dependencies. export const DIRECT_DEP_SELECTOR_WEIGHT = 1000 +// This weight is set for concrete versions of dependencies preexisting in the +// wanted lockfile. When adding a dependency, prefer existing versions first. +// +// This needs to be a higher weight than DIRECT_DEP_SELECTOR_WEIGHT since direct +// dependency specifiers can match a range of versions. Versions on the registry +// not present in the lockfile should be considered at a lower weight than +// matching pre-existing versions. If this is not the case, pnpm could suddenly +// introduce a new version in the lockfile when an existing version works. +export const EXISTING_VERSION_SELECTOR_WEIGHT = 1_000_000 + export type VersionSelectorType = 'version' | 'range' | 'tag' export interface VersionSelectors {