fix(npm): cache bust npm specifiers more aggressively (#18636)

Part 1: #18622 
Part 2: This PR

Closes #16901

---------

Co-authored-by: Luca Casonato <hello@lcas.dev>

4 files changed, 87 insertions, 47 deletions

diff --git a/cli/npm/installer.rs b/cli/npm/installer.rs
index 41d85a9b9..5a15494ab 100644
--- a/cli/npm/installer.rs
+++ b/cli/npm/installer.rs
@@ -1,33 +1,69 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
+use std::future::Future;
 use std::sync::Arc;
 
 use deno_core::error::AnyError;
 use deno_core::futures::stream::FuturesOrdered;
 use deno_core::futures::StreamExt;
 use deno_npm::registry::NpmRegistryApi;
+use deno_npm::registry::NpmRegistryPackageInfoLoadError;
+use deno_semver::npm::NpmPackageReq;
 
 use crate::args::package_json::PackageJsonDeps;
 use crate::util::sync::AtomicFlag;
 
-use super::NpmRegistry;
+use super::CliNpmRegistryApi;
 use super::NpmResolution;
 
 #[derive(Debug)]
 struct PackageJsonDepsInstallerInner {
   has_installed_flag: AtomicFlag,
-  npm_registry_api: NpmRegistry,
+  npm_registry_api: CliNpmRegistryApi,
   npm_resolution: NpmResolution,
   package_deps: PackageJsonDeps,
 }
 
+impl PackageJsonDepsInstallerInner {
+  pub fn reqs(&self) -> Vec<&NpmPackageReq> {
+    let mut package_reqs = self
+      .package_deps
+      .values()
+      .filter_map(|r| r.as_ref().ok())
+      .collect::<Vec<_>>();
+    package_reqs.sort(); // deterministic resolution
+    package_reqs
+  }
+
+  pub fn reqs_with_info_futures(
+    &self,
+  ) -> FuturesOrdered<
+    impl Future<
+      Output = Result<
+        (&NpmPackageReq, Arc<deno_npm::registry::NpmPackageInfo>),
+        NpmRegistryPackageInfoLoadError,
+      >,
+    >,
+  > {
+    let package_reqs = self.reqs();
+
+    FuturesOrdered::from_iter(package_reqs.into_iter().map(|req| {
+      let api = self.npm_registry_api.clone();
+      async move {
+        let info = api.package_info(&req.name).await?;
+        Ok::<_, NpmRegistryPackageInfoLoadError>((req, info))
+      }
+    }))
+  }
+}
+
 /// Holds and controls installing dependencies from package.json.
 #[derive(Debug, Clone, Default)]
 pub struct PackageJsonDepsInstaller(Option<Arc<PackageJsonDepsInstallerInner>>);
 
 impl PackageJsonDepsInstaller {
   pub fn new(
-    npm_registry_api: NpmRegistry,
+    npm_registry_api: CliNpmRegistryApi,
     npm_resolution: NpmResolution,
     deps: Option<PackageJsonDeps>,
   ) -> Self {
@@ -57,27 +93,23 @@ impl PackageJsonDepsInstaller {
       return Ok(()); // already installed by something else
     }
 
-    let mut package_reqs = inner
-      .package_deps
-      .values()
-      .filter_map(|r| r.as_ref().ok())
-      .collect::<Vec<_>>();
-    package_reqs.sort(); // deterministic resolution
-
-    let mut req_with_infos =
-      FuturesOrdered::from_iter(package_reqs.into_iter().map(|req| {
-        let api = inner.npm_registry_api.clone();
-        async move {
-          let info = api.package_info(&req.name).await?;
-          Ok::<_, AnyError>((req, info))
-        }
-      }));
+    let mut reqs_with_info_futures = inner.reqs_with_info_futures();
 
-    while let Some(result) = req_with_infos.next().await {
+    while let Some(result) = reqs_with_info_futures.next().await {
       let (req, info) = result?;
-      inner
+      let result = inner
         .npm_resolution
-        .resolve_package_req_as_pending_with_info(req, &info)?;
+        .resolve_package_req_as_pending_with_info(req, &info);
+      if let Err(err) = result {
+        if inner.npm_registry_api.mark_force_reload() {
+          log::debug!("Failed to resolve package. Retrying. Error: {err:#}");
+          // re-initialize
+          inner.npm_registry_api.clear_memory_cache();
+          reqs_with_info_futures = inner.reqs_with_info_futures();
+        } else {
+          return Err(err.into());
+        }
+      }
     }
 
     Ok(())
diff --git a/cli/npm/mod.rs b/cli/npm/mod.rs
index 95a0a3017..8433a8f0c 100644
--- a/cli/npm/mod.rs
+++ b/cli/npm/mod.rs
@@ -10,7 +10,7 @@ mod tarball;
 pub use cache::should_sync_download;
 pub use cache::NpmCache;
 pub use installer::PackageJsonDepsInstaller;
-pub use registry::NpmRegistry;
+pub use registry::CliNpmRegistryApi;
 pub use resolution::NpmResolution;
 pub use resolvers::create_npm_fs_resolver;
 pub use resolvers::NpmPackageResolver;
diff --git a/cli/npm/registry.rs b/cli/npm/registry.rs
index a87365c9c..b38cfa898 100644
--- a/cli/npm/registry.rs
+++ b/cli/npm/registry.rs
@@ -53,9 +53,9 @@ static NPM_REGISTRY_DEFAULT_URL: Lazy<Url> = Lazy::new(|| {
 });
 
 #[derive(Clone, Debug)]
-pub struct NpmRegistry(Option<Arc<NpmRegistryApiInner>>);
+pub struct CliNpmRegistryApi(Option<Arc<CliNpmRegistryApiInner>>);
 
-impl NpmRegistry {
+impl CliNpmRegistryApi {
   pub fn default_url() -> &'static Url {
     &NPM_REGISTRY_DEFAULT_URL
   }
@@ -66,7 +66,7 @@ impl NpmRegistry {
     http_client: HttpClient,
     progress_bar: ProgressBar,
   ) -> Self {
-    Self(Some(Arc::new(NpmRegistryApiInner {
+    Self(Some(Arc::new(CliNpmRegistryApiInner {
       base_url,
       cache,
       force_reload_flag: Default::default(),
@@ -104,14 +104,18 @@ impl NpmRegistry {
   ///
   /// Returns true if it was successfully set for the first time.
   pub fn mark_force_reload(&self) -> bool {
-    // never force reload the registry information if reloading is disabled
-    if matches!(self.inner().cache.cache_setting(), CacheSetting::Only) {
+    // never force reload the registry information if reloading
+    // is disabled or if we're already reloading
+    if matches!(
+      self.inner().cache.cache_setting(),
+      CacheSetting::Only | CacheSetting::ReloadAll
+    ) {
       return false;
     }
     self.inner().force_reload_flag.raise()
   }
 
-  fn inner(&self) -> &Arc<NpmRegistryApiInner> {
+  fn inner(&self) -> &Arc<CliNpmRegistryApiInner> {
     // this panicking indicates a bug in the code where this
     // wasn't initialized
     self.0.as_ref().unwrap()
@@ -122,7 +126,7 @@ static SYNC_DOWNLOAD_TASK_QUEUE: Lazy<TaskQueue> =
   Lazy::new(TaskQueue::default);
 
 #[async_trait]
-impl NpmRegistryApi for NpmRegistry {
+impl NpmRegistryApi for CliNpmRegistryApi {
   async fn package_info(
     &self,
     name: &str,
@@ -147,16 +151,17 @@ impl NpmRegistryApi for NpmRegistry {
   }
 }
 
+type CacheItemPendingResult =
+  Result<Option<Arc<NpmPackageInfo>>, Arc<AnyError>>;
+
 #[derive(Debug)]
 enum CacheItem {
-  Pending(
-    Shared<BoxFuture<'static, Result<Option<Arc<NpmPackageInfo>>, String>>>,
-  ),
+  Pending(Shared<BoxFuture<'static, CacheItemPendingResult>>),
   Resolved(Option<Arc<NpmPackageInfo>>),
 }
 
 #[derive(Debug)]
-struct NpmRegistryApiInner {
+struct CliNpmRegistryApiInner {
   base_url: Url,
   cache: NpmCache,
   force_reload_flag: AtomicFlag,
@@ -166,7 +171,7 @@ struct NpmRegistryApiInner {
   progress_bar: ProgressBar,
 }
 
-impl NpmRegistryApiInner {
+impl CliNpmRegistryApiInner {
   pub async fn maybe_package_info(
     self: &Arc<Self>,
     name: &str,
@@ -196,9 +201,15 @@ impl NpmRegistryApiInner {
           let future = {
             let api = self.clone();
             let name = name.to_string();
-            async move { api.load_package_info_from_registry(&name).await }
-              .boxed()
-              .shared()
+            async move {
+              api
+                .load_package_info_from_registry(&name)
+                .await
+                .map(|info| info.map(Arc::new))
+                .map_err(Arc::new)
+            }
+            .boxed()
+            .shared()
           };
           mem_cache
             .insert(name.to_string(), CacheItem::Pending(future.clone()));
@@ -220,11 +231,11 @@ impl NpmRegistryApiInner {
         Err(err) => {
           // purge the item from the cache so it loads next time
           self.mem_cache.lock().remove(name);
-          Err(anyhow!("{}", err))
+          Err(anyhow!("{:#}", err))
         }
       }
     } else {
-      Ok(future.await.map_err(|err| anyhow!("{}", err))?)
+      Ok(future.await.map_err(|err| anyhow!("{:#}", err))?)
     }
   }
 
@@ -303,7 +314,7 @@ impl NpmRegistryApiInner {
   async fn load_package_info_from_registry(
     &self,
     name: &str,
-  ) -> Result<Option<Arc<NpmPackageInfo>>, String> {
+  ) -> Result<Option<NpmPackageInfo>, AnyError> {
     self
       .load_package_info_from_registry_inner(name)
       .await
@@ -314,9 +325,6 @@ impl NpmRegistryApiInner {
           name
         )
       })
-      .map(|info| info.map(Arc::new))
-      // make cloneable
-      .map_err(|err| format!("{err:#}"))
   }
 
   async fn load_package_info_from_registry_inner(
diff --git a/cli/npm/resolution.rs b/cli/npm/resolution.rs
index d012b4f08..7d1619b94 100644
--- a/cli/npm/resolution.rs
+++ b/cli/npm/resolution.rs
@@ -27,7 +27,7 @@ use deno_semver::npm::NpmPackageReqReference;
 
 use crate::args::Lockfile;
 
-use super::registry::NpmRegistry;
+use super::registry::CliNpmRegistryApi;
 
 /// Handles updating and storing npm resolution in memory where the underlying
 /// snapshot can be updated concurrently. Additionally handles updating the lockfile
@@ -38,7 +38,7 @@ use super::registry::NpmRegistry;
 pub struct NpmResolution(Arc<NpmResolutionInner>);
 
 struct NpmResolutionInner {
-  api: NpmRegistry,
+  api: CliNpmRegistryApi,
   snapshot: RwLock<NpmResolutionSnapshot>,
   update_queue: TaskQueue,
   maybe_lockfile: Option<Arc<Mutex<Lockfile>>>,
@@ -55,7 +55,7 @@ impl std::fmt::Debug for NpmResolution {
 
 impl NpmResolution {
   pub fn new(
-    api: NpmRegistry,
+    api: CliNpmRegistryApi,
     initial_snapshot: Option<NpmResolutionSnapshot>,
     maybe_lockfile: Option<Arc<Mutex<Lockfile>>>,
   ) -> Self {
@@ -247,7 +247,7 @@ impl NpmResolution {
 }
 
 async fn add_package_reqs_to_snapshot(
-  api: &NpmRegistry,
+  api: &CliNpmRegistryApi,
   // todo(18079): it should be possible to pass &[NpmPackageReq] in here
   // and avoid all these clones, but the LSP complains because of its
   // `Send` requirement