From d5f0dd7ca29a89049b83ea9fc5037cc03d00f56c Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 9 Jun 2025 19:22:12 -0400 Subject: [PATCH] fix(install): bust packument cache on version not found when loading extra info from lockfile (#29679) Co-authored-by: nathanwhit --- resolvers/npm_cache/registry_info.rs | 90 +++++++++---------- resolvers/npm_installer/extra_info.rs | 23 ++++- .../__test__.jsonc | 15 ++++ .../package.json | 2 + .../cache_out_of_date_with_lockfile/update.ts | 16 ++++ 5 files changed, 98 insertions(+), 48 deletions(-) create mode 100644 tests/specs/install/cache_out_of_date_with_lockfile/__test__.jsonc create mode 100644 tests/specs/install/cache_out_of_date_with_lockfile/package.json create mode 100644 tests/specs/install/cache_out_of_date_with_lockfile/update.ts diff --git a/resolvers/npm_cache/registry_info.rs b/resolvers/npm_cache/registry_info.rs index 152c1be8aa..910af7b225 100644 --- a/resolvers/npm_cache/registry_info.rs +++ b/resolvers/npm_cache/registry_info.rs @@ -226,7 +226,7 @@ impl self: &Arc, name: &str, ) -> Result>, LoadPackageInfoInnerError> { - let (cache_item, clear_id) = { + let (value_creator, clear_id) = { let mut mem_cache = self.memory_cache.lock(); let cache_item = if let Some(cache_item) = mem_cache.get(name) { cache_item.clone() @@ -240,53 +240,53 @@ impl mem_cache.insert(name.to_string(), cache_item.clone()); cache_item }; - (cache_item, mem_cache.clear_id) + match cache_item { + MemoryCacheItem::FsCached(info) => return Ok(Some(info)), + MemoryCacheItem::MemoryCached(maybe_info) => { + return maybe_info.map_err(LoadPackageInfoInnerError) + } + MemoryCacheItem::Pending(value_creator) => { + (value_creator, mem_cache.clear_id) + } + } }; - match cache_item { - MemoryCacheItem::FsCached(info) => Ok(Some(info)), - MemoryCacheItem::MemoryCached(maybe_info) => { - maybe_info.clone().map_err(LoadPackageInfoInnerError) + match value_creator.get().await { + Ok(FutureResult::SavedFsCache(info)) => { + // return back the future and mark this package as having + // been saved in the cache for next time it's requested + self.memory_cache.lock().try_insert( + clear_id, + name, + MemoryCacheItem::FsCached(info.clone()), + ); + Ok(Some(info)) } - MemoryCacheItem::Pending(value_creator) => { - match value_creator.get().await { - Ok(FutureResult::SavedFsCache(info)) => { - // return back the future and mark this package as having - // been saved in the cache for next time it's requested - self.memory_cache.lock().try_insert( - clear_id, - name, - MemoryCacheItem::FsCached(info.clone()), - ); - Ok(Some(info)) - } - Ok(FutureResult::ErroredFsCache(info)) => { - // since saving to the fs cache failed, keep the package information in memory - self.memory_cache.lock().try_insert( - clear_id, - name, - MemoryCacheItem::MemoryCached(Ok(Some(info.clone()))), - ); - Ok(Some(info)) - } - Ok(FutureResult::PackageNotExists) => { - self.memory_cache.lock().try_insert( - clear_id, - name, - MemoryCacheItem::MemoryCached(Ok(None)), - ); - Ok(None) - } - Err(err) => { - let return_err = err.clone(); - self.memory_cache.lock().try_insert( - clear_id, - name, - MemoryCacheItem::MemoryCached(Err(err)), - ); - Err(LoadPackageInfoInnerError(return_err)) - } - } + Ok(FutureResult::ErroredFsCache(info)) => { + // since saving to the fs cache failed, keep the package information in memory + self.memory_cache.lock().try_insert( + clear_id, + name, + MemoryCacheItem::MemoryCached(Ok(Some(info.clone()))), + ); + Ok(Some(info)) + } + Ok(FutureResult::PackageNotExists) => { + self.memory_cache.lock().try_insert( + clear_id, + name, + MemoryCacheItem::MemoryCached(Ok(None)), + ); + Ok(None) + } + Err(err) => { + let return_err = err.clone(); + self.memory_cache.lock().try_insert( + clear_id, + name, + MemoryCacheItem::MemoryCached(Err(err)), + ); + Err(LoadPackageInfoInnerError(return_err)) } } } diff --git a/resolvers/npm_installer/extra_info.rs b/resolvers/npm_installer/extra_info.rs index ec1fa7ed22..c8fb539569 100644 --- a/resolvers/npm_installer/extra_info.rs +++ b/resolvers/npm_installer/extra_info.rs @@ -134,14 +134,31 @@ impl NpmPackageExtraInfoProvider { &self, package_nv: &PackageNv, ) -> Result { - let package_info = self + let mut package_info = self .npm_registry_info_provider .package_info(&package_nv.name) .await .map_err(JsErrorBox::from_err)?; - let version_info = package_info + let version_info = match package_info .version_info(package_nv, &self.workspace_patch_packages.0) - .map_err(JsErrorBox::from_err)?; + { + Ok(version_info) => version_info, + Err(deno_npm::resolution::NpmPackageVersionNotFound { .. }) => { + // Don't bother checking the return value of mark_force_reload to tell + // whether to reload because we could race here with another task within + // this method. That said, ideally this code would only reload the + // specific packument that's out of date to be a bit more efficient. + self.npm_registry_info_provider.mark_force_reload(); + package_info = self + .npm_registry_info_provider + .package_info(&package_nv.name) + .await + .map_err(JsErrorBox::from_err)?; + package_info + .version_info(package_nv, &self.workspace_patch_packages.0) + .map_err(JsErrorBox::from_err)? + } + }; Ok(NpmPackageExtraInfo { deprecated: version_info.deprecated.clone(), bin: version_info.bin.clone(), diff --git a/tests/specs/install/cache_out_of_date_with_lockfile/__test__.jsonc b/tests/specs/install/cache_out_of_date_with_lockfile/__test__.jsonc new file mode 100644 index 0000000000..de8d89465c --- /dev/null +++ b/tests/specs/install/cache_out_of_date_with_lockfile/__test__.jsonc @@ -0,0 +1,15 @@ +{ + "tempDir": true, + "steps": [{ + // this would only occur with a lockfile and a package with "extra info" (bin or script) + "args": "install npm:@denotest/bin", + "output": "[WILDCARD]" + }, { + "args": "run -A update.ts $DENO_DIR", + "output": "" + }, { + // should not error + "args": "install", + "output": "[WILDCARD]" + }] +} diff --git a/tests/specs/install/cache_out_of_date_with_lockfile/package.json b/tests/specs/install/cache_out_of_date_with_lockfile/package.json new file mode 100644 index 0000000000..2c63c08510 --- /dev/null +++ b/tests/specs/install/cache_out_of_date_with_lockfile/package.json @@ -0,0 +1,2 @@ +{ +} diff --git a/tests/specs/install/cache_out_of_date_with_lockfile/update.ts b/tests/specs/install/cache_out_of_date_with_lockfile/update.ts new file mode 100644 index 0000000000..16c8594a7d --- /dev/null +++ b/tests/specs/install/cache_out_of_date_with_lockfile/update.ts @@ -0,0 +1,16 @@ +const path = Deno.args[0] + "/npm/localhost_4260/@denotest/bin/registry.json"; +const fileText = Deno.readTextFileSync(path); +const data = JSON.parse(fileText); +if (data.versions["1.0.0"] == null || data["dist-tags"].latest !== "1.0.0") { + throw new Error("Test relies on version 1.0.0 to be the latest version"); +} +delete data.versions["1.0.0"]; +data["dist-tags"].latest = "0.5.0"; +delete data["_deno.etag"]; +Deno.writeTextFileSync(path, JSON.stringify(data)); +Deno.remove("node_modules", { recursive: true }); + +// assert this exists +if (!Deno.statSync("deno.lock").isFile) { + throw new Error("Expected a deno.lock file."); +}