From bd578478333bf1b695b00e633e7cb5906b9646eb Mon Sep 17 00:00:00 2001 From: elrrrrrrr Date: Thu, 16 Apr 2026 20:17:53 +0800 Subject: [PATCH 1/3] fix(pm): detect stale lockfile in ut install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fs::try_exists(&lock_path)` check in Self::install. As a result any `ut install` with an existing package-lock.json skipped the manifest resolution entirely — even when package.json had been edited — silently installing the stale tree from the lockfile. Re-introduce the freshness check: when the lockfile exists we now still run `is_pkg_lock_outdated`. Stale lockfiles (or freshness-check errors — corrupt lockfiles etc.) fall through to the pipeline cold path so we keep the concurrent resolve + download benefit #2576 was built for. Regression guard lives in `e2e/pm/stale-lockfile/` — a fixture that commits a package-lock.json whose root `dependencies` is empty while package.json declares abbrev and ini. Case 8.2 in e2e/utoo-pm.sh asserts: - `ut install` regenerates the lockfile - both packages end up in node_modules/ Co-Authored-By: Claude Opus 4.6 (1M context) --- crates/pm/src/service/install.rs | 17 +++++++--- e2e/pm/stale-lockfile/package-lock.json | 13 ++++++++ e2e/pm/stale-lockfile/package.json | 10 ++++++ e2e/utoo-pm.sh | 43 +++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 5 deletions(-) create mode 100644 e2e/pm/stale-lockfile/package-lock.json create mode 100644 e2e/pm/stale-lockfile/package.json diff --git a/crates/pm/src/service/install.rs b/crates/pm/src/service/install.rs index 738c1026fd..8c679a9d22 100644 --- a/crates/pm/src/service/install.rs +++ b/crates/pm/src/service/install.rs @@ -8,7 +8,7 @@ use crate::cmd::deps::build_deps; use crate::fs; use crate::helper::global_bin::get_global_bin_dir; use crate::helper::lock::{ - Package, UpdatePackageJsonOptions, extract_package_name, group_by_depth, + Package, UpdatePackageJsonOptions, extract_package_name, group_by_depth, is_pkg_lock_outdated, prepare_global_package_json, update_package_json, }; use crate::helper::workspace::init_project_root; @@ -224,14 +224,21 @@ impl InstallService { omit: &HashSet, ) -> Result<()> { let lock_path = root_path.join("package-lock.json"); + let lock_exists = fs::try_exists(&lock_path).await.unwrap_or(false); - let (package_lock, pipeline_handles) = if fs::try_exists(&lock_path).await.unwrap_or(false) - { - // Lock exists: install directly, skip manifest resolution + // An existing lockfile is trusted only when it still matches + // package.json. A failure to answer the freshness check (corrupt + // lockfile, unreadable package.json, …) is treated as stale so we + // regenerate rather than install from a file we can't validate. + let lock_stale = lock_exists && is_pkg_lock_outdated(root_path).await.unwrap_or(true); + + let (package_lock, pipeline_handles) = if lock_exists && !lock_stale { let lock = load_package_lock_json_from_path(root_path).await?; (lock, None) } else { - // No lock: full pipeline flow (resolve + concurrent download/clone) + if lock_stale { + tracing::warn!("package-lock.json is outdated, regenerating"); + } start_progress_bar(); let result = super::pipeline::resolve_with_pipeline(root_path).await?; finish_progress_bar("package-lock.json resolved"); diff --git a/e2e/pm/stale-lockfile/package-lock.json b/e2e/pm/stale-lockfile/package-lock.json new file mode 100644 index 0000000000..8d4bce1324 --- /dev/null +++ b/e2e/pm/stale-lockfile/package-lock.json @@ -0,0 +1,13 @@ +{ + "name": "e2e-stale-lockfile", + "version": "1.0.0", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "name": "e2e-stale-lockfile", + "version": "1.0.0", + "dependencies": {} + } + } +} diff --git a/e2e/pm/stale-lockfile/package.json b/e2e/pm/stale-lockfile/package.json new file mode 100644 index 0000000000..5be1caa0bd --- /dev/null +++ b/e2e/pm/stale-lockfile/package.json @@ -0,0 +1,10 @@ +{ + "name": "e2e-stale-lockfile", + "version": "1.0.0", + "private": true, + "description": "e2e: package.json declares deps the lockfile hasn't been resolved for yet", + "dependencies": { + "abbrev": "^2.0.0", + "ini": "^5.0.0" + } +} diff --git a/e2e/utoo-pm.sh b/e2e/utoo-pm.sh index 3782c619a3..a3ff0a55b1 100755 --- a/e2e/utoo-pm.sh +++ b/e2e/utoo-pm.sh @@ -182,6 +182,49 @@ done echo -e "${GREEN}PASS: HTTP tarball warm install successful${NC}" cd ../../.. +# Case 8.4: stale lockfile is detected and regenerated on `ut install` +# +# package.json declares two deps; we seed an empty lockfile. A correct +# `ut install` must notice the mismatch, re-resolve, and install both. +# Regression guard for #2576. +echo -e "${YELLOW}Case 8.4: stale lockfile detection${NC}" +cd e2e/pm/stale-lockfile +rm -rf node_modules +cat > package-lock.json <<'JSON' +{ + "name": "e2e-stale-lockfile", + "version": "1.0.0", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "name": "e2e-stale-lockfile", + "version": "1.0.0", + "dependencies": {} + } + } +} +JSON +utoo install --ignore-scripts || { echo -e "${RED}FAIL: utoo install failed on stale lockfile fixture${NC}"; exit 1; } +for pkg in abbrev ini; do + if [ ! -f "node_modules/$pkg/package.json" ]; then + echo -e "${RED}FAIL: $pkg not installed (stale lockfile not regenerated)${NC}"; exit 1 + fi +done +# Assert the ROOT entry's dependencies got rewritten — grep would match +# transitive node_modules/... entries even if the regen failed. +node -e ' +const lock = require("./package-lock.json"); +const root = (lock.packages || {})[""] || {}; +const deps = root.dependencies || {}; +if (!deps.abbrev || !deps.ini) { + console.error("root deps after install:", deps); + process.exit(1); +} +' || { echo -e "${RED}FAIL: package-lock.json root deps not regenerated${NC}"; exit 1; } +echo -e "${GREEN}PASS: stale lockfile detected + regenerated${NC}" +cd ../../.. + # Case 9: reinstall ant-design by npmjs.org echo -e "${YELLOW}Case 9: reinstall ant-design${NC} by npmjs.org" cd e2e/pm/ant-design/ant-design From e77bc16c971555c186faabb6f668b190f1b099fd Mon Sep 17 00:00:00 2001 From: elrrrrrrr Date: Thu, 16 Apr 2026 20:25:33 +0800 Subject: [PATCH 2/3] refactor(pm): simplify stale-lockfile detection per review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Drop duplicate `tracing::warn` in Self::install; `is_pkg_lock_outdated` already logs the specific mismatch reason (root deps changed, engines changed, …). Users now see one warn instead of two per stale detection. - Collapse `lock_exists` + `lock_stale` into a single `use_fresh_lock` expression — removes the double-negation. - Drop the committed stale `package-lock.json` fixture and the `git checkout --` dance in Case 8.2. The e2e now writes the stale lockfile inline via heredoc, which: * survives local edits / worktrees / stashed state * avoids the `git add -f` fight with .gitignore * keeps the fixture dir to just package.json (the declarative half) - Assert root deps via `node` inspection of `packages[""].dependencies` instead of `grep "abbrev"`, which would match transitive `node_modules/abbrev` entries and hide a regression where only the tree — not the root declaration — got rewritten. Co-Authored-By: Claude Opus 4.6 (1M context) --- crates/pm/src/service/install.rs | 17 ++++++----------- e2e/pm/stale-lockfile/package-lock.json | 13 ------------- 2 files changed, 6 insertions(+), 24 deletions(-) delete mode 100644 e2e/pm/stale-lockfile/package-lock.json diff --git a/crates/pm/src/service/install.rs b/crates/pm/src/service/install.rs index 8c679a9d22..e4591d3b38 100644 --- a/crates/pm/src/service/install.rs +++ b/crates/pm/src/service/install.rs @@ -224,21 +224,16 @@ impl InstallService { omit: &HashSet, ) -> Result<()> { let lock_path = root_path.join("package-lock.json"); - let lock_exists = fs::try_exists(&lock_path).await.unwrap_or(false); + // Treat a failing freshness check as stale: regenerate rather than + // install from a lockfile we couldn't validate. `is_pkg_lock_outdated` + // itself emits a `tracing::warn` with the specific mismatch reason. + let use_fresh_lock = fs::try_exists(&lock_path).await.unwrap_or(false) + && !is_pkg_lock_outdated(root_path).await.unwrap_or(true); - // An existing lockfile is trusted only when it still matches - // package.json. A failure to answer the freshness check (corrupt - // lockfile, unreadable package.json, …) is treated as stale so we - // regenerate rather than install from a file we can't validate. - let lock_stale = lock_exists && is_pkg_lock_outdated(root_path).await.unwrap_or(true); - - let (package_lock, pipeline_handles) = if lock_exists && !lock_stale { + let (package_lock, pipeline_handles) = if use_fresh_lock { let lock = load_package_lock_json_from_path(root_path).await?; (lock, None) } else { - if lock_stale { - tracing::warn!("package-lock.json is outdated, regenerating"); - } start_progress_bar(); let result = super::pipeline::resolve_with_pipeline(root_path).await?; finish_progress_bar("package-lock.json resolved"); diff --git a/e2e/pm/stale-lockfile/package-lock.json b/e2e/pm/stale-lockfile/package-lock.json deleted file mode 100644 index 8d4bce1324..0000000000 --- a/e2e/pm/stale-lockfile/package-lock.json +++ /dev/null @@ -1,13 +0,0 @@ -{ - "name": "e2e-stale-lockfile", - "version": "1.0.0", - "lockfileVersion": 3, - "requires": true, - "packages": { - "": { - "name": "e2e-stale-lockfile", - "version": "1.0.0", - "dependencies": {} - } - } -} From 33ca6e6295731300e4c77da5728bdcb9a3b7ad50 Mon Sep 17 00:00:00 2001 From: elrrrrrrr Date: Thu, 16 Apr 2026 20:50:23 +0800 Subject: [PATCH 3/3] perf(pm): reuse cached package.json in is_pkg_lock_outdated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The freshness check was reading the root package.json three times per `ut install` — once as DepsView, once inside find_workspaces (for the "workspaces" field), once as EnginesView. Each workspace was also read twice (once by find_workspaces, again as DepsView). Load the root as full `PackageJson` via `get_or_load_package_json` so subsequent reads on the same path hit the in-process cache (filled by `update_package_json`'s write-through). Within this function, drop the DepsView/EnginesView intermediates and read the fields directly off the same `PackageJson`; workspaces come back pre-loaded on the `find_workspaces` tuple so we don't re-load them either. Cache coherence: `update_package_json` already calls `set_package_json` write-through after every edit, and every `ut install` starts a fresh process with a cold cache — so external edits (IDE, `git checkout`) still see a disk read next run. Tests: the original `test_is_pkg_lock_outdated` simulated a "write file, re-check" flow that doesn't happen in real invocations (every mutation goes via `update_package_json` + write-through). Split it into five independent scenarios, each with its own TempDir so cache keys don't collide, which is also the idiomatic "one test per case". Co-Authored-By: Claude Opus 4.6 (1M context) --- crates/pm/src/helper/lock.rs | 245 ++++++++++++++++++----------------- 1 file changed, 123 insertions(+), 122 deletions(-) diff --git a/crates/pm/src/helper/lock.rs b/crates/pm/src/helper/lock.rs index 65a9f00839..f9969db1db 100644 --- a/crates/pm/src/helper/lock.rs +++ b/crates/pm/src/helper/lock.rs @@ -4,7 +4,7 @@ use std::path::{Path, PathBuf}; use anyhow::{Context as _, Result, anyhow}; use serde_json::Value; use utoo_ruborist::lock::{LockPackage, PackageLock}; -use utoo_ruborist::manifest::{DepsView, EnginesView, PackageJson}; +use utoo_ruborist::manifest::PackageJson; use utoo_ruborist::registry::resolve_package; use utoo_ruborist::spec::{PackageSpec, Protocol, resolve_catalog_spec}; use utoo_ruborist::util::PackageNameStr; @@ -17,12 +17,14 @@ use crate::util::cli_enum::{PackageAction, SaveType}; use crate::util::cloner::clone_package; use crate::util::downloader::{is_git_url, resolve_cache_path}; use crate::util::git_resolver::{resolve_git_spec, resolve_github_spec}; -use crate::util::json::{load_package_json, load_package_lock_json_from_path, read_json_file}; +use crate::util::json::{load_package_lock_json_from_path, read_json_file}; use crate::util::logger::{finish_progress_bar, start_progress_bar}; use utoo_ruborist::builder::PeerDeps; use crate::util::platform_const::GLOBAL_NODE_MODULES; -use crate::util::user_config::{get_catalogs, get_peer_deps, set_package_json}; +use crate::util::user_config::{ + get_catalogs, get_or_load_package_json, get_peer_deps, set_package_json, +}; // Platform-specific line endings #[cfg(target_os = "windows")] @@ -349,16 +351,22 @@ pub fn path_to_pkg_name(path_str: &str) -> Option<&str> { } pub async fn is_pkg_lock_outdated(root_path: &Path) -> Result { - // Always read from disk — this function checks whether the on-disk - // package.json has diverged from the lock file, so it must not use the - // in-process cache. - let pkg: DepsView = load_package_json(root_path).await?; + // Root package.json is served from the in-process cache. The cache + // stays consistent with disk across the full `ut install` lifetime + // because `update_package_json` calls `set_package_json` write-through + // after every edit, and a fresh `ut install` process starts with a + // cold cache (so external edits / git checkout always see a re-read). + // The lockfile itself is still read from disk each time — it's the + // thing whose freshness we're checking. + let root_pkg = get_or_load_package_json(root_path).await?; let lock_file: PackageLock = read_json_file(&root_path.join("package-lock.json")).await?; let catalogs = get_catalogs().await; - let deps_match = |pkg_deps: &HashMap, + let deps_match = |pkg_deps: Option<&HashMap>, lock_deps: Option<&HashMap>| -> bool { + let empty = HashMap::new(); + let pkg_deps = pkg_deps.unwrap_or(&empty); match lock_deps { None => pkg_deps.is_empty(), Some(ld) => { @@ -372,24 +380,20 @@ pub async fn is_pkg_lock_outdated(root_path: &Path) -> Result { }; let packages = &lock_file.packages; + let peer_deps = get_peer_deps().await; - // prepare packages to check: (relative_path, deps) - let mut pkgs_to_check: Vec<(String, DepsView)> = vec![("".to_string(), pkg)]; - - // populate all workspaces (load only the deps view for each) let workspaces = find_workspaces(root_path).await?; - for (_, path, _) in workspaces { + let mut pkgs_to_check: Vec<(String, &PackageJson)> = Vec::with_capacity(1 + workspaces.len()); + pkgs_to_check.push((String::new(), &root_pkg)); + for (_, path, pkg) in &workspaces { let target_path = path .strip_prefix(root_path) - .unwrap_or(&path) + .unwrap_or(path) .to_string_lossy() .to_string(); - let ws_deps: DepsView = load_package_json(&path).await?; - pkgs_to_check.push((target_path, ws_deps)); + pkgs_to_check.push((target_path, pkg)); } - let peer_deps = get_peer_deps().await; - for (path, pkg) in &pkgs_to_check { let lock = match packages.get(path.as_str()) { Some(lock) => lock, @@ -402,40 +406,43 @@ pub async fn is_pkg_lock_outdated(root_path: &Path) -> Result { let name = if path.is_empty() { "root" } else { path }; - // check dependencies whether changed - if !deps_match(&pkg.dependencies, lock.dependencies.as_ref()) { + if !deps_match(pkg.dependencies.as_ref(), lock.dependencies.as_ref()) { tracing::warn!("package-lock.json is outdated, {name} dependencies changed"); return Ok(true); } if !deps_match( - &pkg.optional_dependencies, + pkg.optional_dependencies.as_ref(), lock.optional_dependencies.as_ref(), ) { tracing::warn!("package-lock.json is outdated, {name} optionalDependencies changed"); return Ok(true); } - if !deps_match(&pkg.dev_dependencies, lock.dev_dependencies.as_ref()) { + if !deps_match( + pkg.dev_dependencies.as_ref(), + lock.dev_dependencies.as_ref(), + ) { tracing::warn!("package-lock.json is outdated, {name} devDependencies changed"); return Ok(true); } if peer_deps == PeerDeps::Include - && !deps_match(&pkg.peer_dependencies, lock.peer_dependencies.as_ref()) + && !deps_match( + pkg.peer_dependencies.as_ref(), + lock.peer_dependencies.as_ref(), + ) { tracing::warn!("package-lock.json is outdated, {name} peerDependencies changed"); return Ok(true); } } - // engines: root package only - let root_engines: EnginesView = load_package_json(root_path).await?; + // engines: root only — derived from the same loaded `root_pkg` let root_lock = packages .get("") .ok_or_else(|| anyhow!("Missing root in package-lock.json"))?; - - let pkg_engines = root_engines.engines.as_ref().filter(|m| !m.is_empty()); + let pkg_engines = root_pkg.engines.as_ref().filter(|m| !m.is_empty()); let lock_engines = root_lock.engines.as_ref().filter(|m| !m.is_empty()); let engines_match = match (pkg_engines, lock_engines) { @@ -560,25 +567,13 @@ mod tests { ); } - #[tokio::test] - async fn test_is_pkg_lock_outdated() { - // Create a temporary directory - let temp_dir = TempDir::new().unwrap(); - let temp_path = temp_dir.path(); - - // Test case 1: package.json and package-lock.json are in sync - let pkg_json = json!({ - "name": "test-package", - "version": "1.0.0", - "dependencies": { - "lodash": "^4.17.20" - }, - "devDependencies": { - "typescript": "^4.9.0" - } - }); - - let pkg_lock = json!({ + /// Baseline lockfile for the outdated-check scenarios. + /// + /// Each scenario uses a fresh `TempDir` (unique path) so the in-process + /// package.json cache never serves a stale entry — which mirrors real + /// `ut install` where every invocation starts with a cold cache. + fn baseline_pkg_lock() -> Value { + json!({ "name": "test-package", "version": "1.0.0", "lockfileVersion": 3, @@ -587,94 +582,100 @@ mod tests { "": { "name": "test-package", "version": "1.0.0", - "dependencies": { - "lodash": "^4.17.20" - }, - "devDependencies": { - "typescript": "^4.9.0" - } + "dependencies": { "lodash": "^4.17.20" }, + "devDependencies": { "typescript": "^4.9.0" } } } - }); + }) + } - // Write test files to temporary directory + async fn assert_outdated(pkg_json: Value, expected: bool) { + let temp_dir = TempDir::new().unwrap(); + let temp_path = temp_dir.path(); fs::write(temp_path.join("package.json"), pkg_json.to_string()).unwrap(); - fs::write(temp_path.join("package-lock.json"), pkg_lock.to_string()).unwrap(); - - // Test that files are in sync - assert!(!is_pkg_lock_outdated(temp_path).await.unwrap()); - - // Test case 2: package.json has new dependency - let pkg_json_updated = json!({ - "name": "test-package", - "version": "1.0.0", - "dependencies": { - "lodash": "^4.17.20", - "react": "^18.0.0" // New dependency - }, - "devDependencies": { - "typescript": "^4.9.0" - } - }); - - fs::write(temp_path.join("package.json"), pkg_json_updated.to_string()).unwrap(); - let outdated = is_pkg_lock_outdated(temp_path).await.unwrap(); - assert!(outdated); - - // Test case 3: package.json has updated version - let pkg_json_version_updated = json!({ - "name": "test-package", - "version": "1.0.0", - "dependencies": { - "lodash": "^4.17.21" // Updated version - }, - "devDependencies": { - "typescript": "^4.9.0" - } - }); - fs::write( - temp_path.join("package.json"), - pkg_json_version_updated.to_string(), + temp_path.join("package-lock.json"), + baseline_pkg_lock().to_string(), ) .unwrap(); - assert!(is_pkg_lock_outdated(temp_path).await.unwrap()); + assert_eq!( + is_pkg_lock_outdated(temp_path).await.unwrap(), + expected, + "package.json:\n{pkg_json}" + ); + } - // Test case 4: package.json has removed dependency - let pkg_json_removed = json!({ - "name": "test-package", - "version": "1.0.0", - "dependencies": { - "lodash": "^4.17.20" - } - // Removed devDependencies - }); + #[tokio::test] + async fn test_is_pkg_lock_outdated_in_sync() { + assert_outdated( + json!({ + "name": "test-package", + "version": "1.0.0", + "dependencies": { "lodash": "^4.17.20" }, + "devDependencies": { "typescript": "^4.9.0" } + }), + false, + ) + .await; + } - fs::write(temp_path.join("package.json"), pkg_json_removed.to_string()).unwrap(); - assert!(is_pkg_lock_outdated(temp_path).await.unwrap()); + #[tokio::test] + async fn test_is_pkg_lock_outdated_new_dep_added() { + assert_outdated( + json!({ + "name": "test-package", + "version": "1.0.0", + "dependencies": { + "lodash": "^4.17.20", + "react": "^18.0.0" + }, + "devDependencies": { "typescript": "^4.9.0" } + }), + true, + ) + .await; + } - // Test case 4: package.json has removed dependency - let pkg_json_engines_changed = json!({ - "name": "test-package", - "version": "1.0.0", - "dependencies": { - "lodash": "^4.17.20" - }, - "devDependencies": { - "typescript": "^4.9.0" - }, - "engines": { - "install-node": "16" - } - // Removed devDependencies - }); + #[tokio::test] + async fn test_is_pkg_lock_outdated_version_bumped() { + assert_outdated( + json!({ + "name": "test-package", + "version": "1.0.0", + "dependencies": { "lodash": "^4.17.21" }, + "devDependencies": { "typescript": "^4.9.0" } + }), + true, + ) + .await; + } - fs::write( - temp_path.join("package.json"), - pkg_json_engines_changed.to_string(), + #[tokio::test] + async fn test_is_pkg_lock_outdated_dev_deps_removed() { + assert_outdated( + json!({ + "name": "test-package", + "version": "1.0.0", + "dependencies": { "lodash": "^4.17.20" } + }), + true, ) - .unwrap(); - assert!(is_pkg_lock_outdated(temp_path).await.unwrap()); + .await; + } + + #[tokio::test] + async fn test_is_pkg_lock_outdated_engines_added() { + assert_outdated( + json!({ + "name": "test-package", + "version": "1.0.0", + "dependencies": { "lodash": "^4.17.20" }, + "devDependencies": { "typescript": "^4.9.0" }, + "engines": { "install-node": "16" } + }), + true, + ) + .await; } #[test]