From 97f049fb6af2b1d23557c52bf3af651055e3479d Mon Sep 17 00:00:00 2001 From: Victor Sumner <308886+vsumner@users.noreply.github.com> Date: Fri, 27 Feb 2026 16:51:08 -0500 Subject: [PATCH] fix: skip re-importing packages when global virtual store is warm (#10709) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: skip re-importing packages when global virtual store is warm When node_modules is deleted but the global virtual store directories survive, pnpm previously re-fetched every package because the skip logic required currentLockfile to be present. Add a fast-path that checks pathExists(dir) for GVS directories even when currentLockfile is missing, since the GVS directory hash encodes engine, integrity, and full dependency subgraph. * fix: remove includeUnchangedDeps guard from GVS fast-path The includeUnchangedDeps flag is true whenever currentHoistPattern differs from the desired hoistPattern. After deleting node_modules, currentHoistPattern is always undefined (read from .modules.yaml), so the flag is always true when hoisting is configured — defeating the optimization in the exact scenario it targets. The guard is unnecessary because the fast-path only skips fetch/import (fetchResponse = {}), not graph inclusion. The package is still added to the graph with children populated, so hoisting recalculation works. * perf: add GVS warm reinstall benchmark scenario Adds benchmark 6: frozen lockfile reinstall with a warm global virtual store after deleting node_modules. This measures the reattach fast-path where all packages are skipped (no fetch/import) because their GVS hash directories already exist. * fix: use proper types in fetchPackage spy to pass tsgo strict checks --- .changeset/global-virtual-store-reattach.md | 6 +++ benchmarks/bench.sh | 45 ++++++++++++++++++- benchmarks/generate-results.js | 1 + deps/graph-builder/src/lockfileToDepGraph.ts | 7 +++ .../core/test/install/globalVirtualStore.ts | 44 ++++++++++++++++++ 5 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 .changeset/global-virtual-store-reattach.md diff --git a/.changeset/global-virtual-store-reattach.md b/.changeset/global-virtual-store-reattach.md new file mode 100644 index 0000000000..ff0aa7ed22 --- /dev/null +++ b/.changeset/global-virtual-store-reattach.md @@ -0,0 +1,6 @@ +--- +"@pnpm/graph-builder": patch +"pnpm": patch +--- + +Skip re-importing packages from the global virtual store when `node_modules` is deleted but the store directories are still warm. The global store directory hash already encodes engine, integrity, and full dependency subgraph, so existence is proof of validity. diff --git a/benchmarks/bench.sh b/benchmarks/bench.sh index 61dba8b3b6..82498ff87f 100755 --- a/benchmarks/bench.sh +++ b/benchmarks/bench.sh @@ -228,6 +228,49 @@ run_bench "cold" \ "rm -rf {project}/node_modules {project}/pnpm-lock.yaml {store} {cache} && cp $BENCH_DIR/original-package.json {project}/package.json" \ "cd {project} && node {bin} install --ignore-scripts --no-frozen-lockfile >/dev/null 2>&1" +# ── Benchmark 6: GVS warm reinstall ─────────────────────────────────────── +# Global virtual store enabled, GVS warm, node_modules deleted. +# This tests the reattach fast-path: all packages should be skipped +# (no fetch/import) because their GVS hash directories already exist. + +echo "" +echo "━━━ Benchmark 6: GVS warm reinstall (frozen lockfile, warm global virtual store) ━━━" + +# Set up separate GVS-enabled project directories per variant +GVS_PROJECTS=() +for i in "${!VARIANTS[@]}"; do + gvs_dir="$BENCH_DIR/project-gvs-${VARIANTS[$i]}" + mkdir -p "$gvs_dir" + cp "$BRANCH_DIR/benchmarks/fixture.package.json" "$gvs_dir/package.json" + printf "storeDir: %s\ncacheDir: %s\nenableGlobalVirtualStore: true\n" \ + "${VARIANT_STORES[$i]}" "${VARIANT_CACHES[$i]}" > "$gvs_dir/pnpm-workspace.yaml" + GVS_PROJECTS+=("$gvs_dir") + + # Warm the GVS with a full install + echo "Warming GVS for ${VARIANTS[$i]}..." + cd "$gvs_dir" && node "${VARIANT_BINS[$i]}" install --ignore-scripts --no-frozen-lockfile >/dev/null 2>&1 + cp "$gvs_dir/pnpm-lock.yaml" "$BENCH_DIR/saved-lockfile-gvs-${VARIANTS[$i]}.yaml" +done + +for i in "${!VARIANTS[@]}"; do + variant="${VARIANTS[$i]}" + gvs_project="${GVS_PROJECTS[$i]}" + bin="${VARIANT_BINS[$i]}" + lockfile="$BENCH_DIR/saved-lockfile-gvs-$variant.yaml" + + echo "" + echo " $variant:" + hyperfine \ + --warmup "$WARMUP" \ + --runs "$RUNS" \ + --ignore-failure \ + --prepare "rm -rf $gvs_project/node_modules && cp $lockfile $gvs_project/pnpm-lock.yaml" \ + --command-name "$variant" \ + "cd $gvs_project && node $bin install --frozen-lockfile --ignore-scripts >/dev/null 2>&1" \ + --export-json "$BENCH_DIR/gvs-warm-${variant}.json" \ + || true +done + # ── Summary ───────────────────────────────────────────────────────────────── RESULTS_MD="$BENCH_DIR/results.md" @@ -239,7 +282,7 @@ echo "" echo "Results saved to: $RESULTS_MD" # Cleanup -for project in "${VARIANT_PROJECTS[@]}"; do +for project in "${VARIANT_PROJECTS[@]}" "${GVS_PROJECTS[@]}"; do rm -rf "$project/node_modules" done echo "" diff --git a/benchmarks/generate-results.js b/benchmarks/generate-results.js index dd6300ce0a..1b62186127 100644 --- a/benchmarks/generate-results.js +++ b/benchmarks/generate-results.js @@ -9,6 +9,7 @@ const benchmarks = [ ['nolockfile', 'Full resolution (warm, no lockfile)'], ['headless-cold', 'Headless (cold store+cache)'], ['cold', 'Cold install (nothing warm)'], + ['gvs-warm', 'GVS warm reinstall (warm global store)'], ] function readResult (benchDir, name, variant) { diff --git a/deps/graph-builder/src/lockfileToDepGraph.ts b/deps/graph-builder/src/lockfileToDepGraph.ts index fdd843cd04..769bbf160a 100644 --- a/deps/graph-builder/src/lockfileToDepGraph.ts +++ b/deps/graph-builder/src/lockfileToDepGraph.ts @@ -256,6 +256,13 @@ async function buildGraphFromPackages ( } } + if (!fetchResponse && opts.enableGlobalVirtualStore && !isDirectoryDep + && !opts.force) { + if (dirExists ?? await pathExists(dir)) { + fetchResponse = {} + } + } + if (!fetchResponse) { const resolution = pkgSnapshotToResolution(depPath, pkgSnapshot, opts.registries) progressLogger.debug({ packageId, requester: opts.lockfileDir, status: 'resolved' }) diff --git a/pkg-manager/core/test/install/globalVirtualStore.ts b/pkg-manager/core/test/install/globalVirtualStore.ts index 933eff006c..defde66a46 100644 --- a/pkg-manager/core/test/install/globalVirtualStore.ts +++ b/pkg-manager/core/test/install/globalVirtualStore.ts @@ -49,6 +49,50 @@ test('using a global virtual store', async () => { } }) +test('reinstall from warm global virtual store after deleting node_modules', async () => { + prepareEmpty() + const globalVirtualStoreDir = path.resolve('links') + const manifest = { + dependencies: { + '@pnpm.e2e/pkg-with-1-dep': '100.0.0', + }, + } + const opts = testDefaults({ + enableGlobalVirtualStore: true, + virtualStoreDir: globalVirtualStoreDir, + hoistPattern: ['*'], + }) + await install(manifest, opts) + + // Delete only node_modules, keep the global virtual store warm + rimraf('node_modules') + expect(fs.existsSync(globalVirtualStoreDir)).toBeTruthy() + + // Spy on fetchPackage to verify the fast-path skips fetching + const originalFetchPackage = opts.storeController.fetchPackage + let fetchPackageCalls = 0 + opts.storeController.fetchPackage = ((fetchOpts) => { + fetchPackageCalls++ + return originalFetchPackage(fetchOpts) + }) as typeof originalFetchPackage + + // Reinstall with frozenLockfile — should reattach from the warm global store + await install(manifest, { + ...opts, + frozenLockfile: true, + }) + + // fetchPackage should NOT be called — all packages reattached from warm GVS + expect(fetchPackageCalls).toBe(0) + + expect(fs.existsSync(path.resolve('node_modules/.pnpm/node_modules/@pnpm.e2e/dep-of-pkg-with-1-dep/package.json'))).toBeTruthy() + expect(fs.existsSync(path.resolve('node_modules/.pnpm/lock.yaml'))).toBeTruthy() + const files = fs.readdirSync(path.join(globalVirtualStoreDir, '@pnpm.e2e/pkg-with-1-dep/100.0.0')) + expect(files).toHaveLength(1) + expect(fs.existsSync(path.join(globalVirtualStoreDir, '@pnpm.e2e/pkg-with-1-dep/100.0.0', files[0], 'node_modules/@pnpm.e2e/pkg-with-1-dep/package.json'))).toBeTruthy() + expect(fs.existsSync(path.join(globalVirtualStoreDir, '@pnpm.e2e/pkg-with-1-dep/100.0.0', files[0], 'node_modules/@pnpm.e2e/dep-of-pkg-with-1-dep/package.json'))).toBeTruthy() +}) + test('modules are correctly updated when using a global virtual store', async () => { prepareEmpty() const globalVirtualStoreDir = path.resolve('links')