fix(install): bust packument cache on version not found when loading extra info from lockfile (#29679)

Co-authored-by: nathanwhit <nathanwhit@users.noreply.github.com>

resolvers/npm_cache/registry_info.rs

@@ -226,7 +226,7 @@ impl<THttpClient: NpmCacheHttpClient, TSys: NpmCacheSys>
     self: &Arc<Self>,
     name: &str,
   ) -> Result<Option<Arc<NpmPackageInfo>>, 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<THttpClient: NpmCacheHttpClient, TSys: NpmCacheSys>
         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 {
+    match value_creator.get().await {
-      MemoryCacheItem::FsCached(info) => Ok(Some(info)),
+      Ok(FutureResult::SavedFsCache(info)) => {
-      MemoryCacheItem::MemoryCached(maybe_info) => {
+        // return back the future and mark this package as having
-        maybe_info.clone().map_err(LoadPackageInfoInnerError)
+        // 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) => {
+      Ok(FutureResult::ErroredFsCache(info)) => {
-        match value_creator.get().await {
+        // since saving to the fs cache failed, keep the package information in memory
-          Ok(FutureResult::SavedFsCache(info)) => {
+        self.memory_cache.lock().try_insert(
-            // return back the future and mark this package as having
+          clear_id,
-            // been saved in the cache for next time it's requested
+          name,
-            self.memory_cache.lock().try_insert(
+          MemoryCacheItem::MemoryCached(Ok(Some(info.clone()))),
-              clear_id,
+        );
-              name,
+        Ok(Some(info))
-              MemoryCacheItem::FsCached(info.clone()),
+      }
-            );
+      Ok(FutureResult::PackageNotExists) => {
-            Ok(Some(info))
+        self.memory_cache.lock().try_insert(
-          }
+          clear_id,
-          Ok(FutureResult::ErroredFsCache(info)) => {
+          name,
-            // since saving to the fs cache failed, keep the package information in memory
+          MemoryCacheItem::MemoryCached(Ok(None)),
-            self.memory_cache.lock().try_insert(
+        );
-              clear_id,
+        Ok(None)
-              name,
+      }
-              MemoryCacheItem::MemoryCached(Ok(Some(info.clone()))),
+      Err(err) => {
-            );
+        let return_err = err.clone();
-            Ok(Some(info))
+        self.memory_cache.lock().try_insert(
-          }
+          clear_id,
-          Ok(FutureResult::PackageNotExists) => {
+          name,
-            self.memory_cache.lock().try_insert(
+          MemoryCacheItem::MemoryCached(Err(err)),
-              clear_id,
+        );
-              name,
+        Err(LoadPackageInfoInnerError(return_err))
-              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))
-          }
-        }
       }
     }
   }

resolvers/npm_installer/extra_info.rs

@@ -134,14 +134,31 @@ impl NpmPackageExtraInfoProvider {
     &self,
     package_nv: &PackageNv,
   ) -> Result<NpmPackageExtraInfo, JsErrorBox> {
-    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(),

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]"
+  }]
+}

tests/specs/install/cache_out_of_date_with_lockfile/package.json

@@ -0,0 +1,2 @@
+{
+}

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.");
+}