fix: cache bust jsr deps on constraint failure (denoland#22372)

Removes the `FileFetcher`'s internal cache because I don't believe it's
necessary (we already cache this kind of stuff in places like deno_graph
or config files in different places). Removing it fixes this bug because
this functionality was already implemented in deno_graph and lowers
memory usage of the CLI a little bit.

Author: dsherret

Cargo.lock

@@ -97,6 +97,7 @@ chrono = { version = "0.4", default-features = false, features = ["std", "serde"
 console_static_text = "=0.8.1"
 data-encoding = "2.3.3"
 data-url = "=0.3.0"
+deno_cache_dir = "=0.6.1"
 dlopen2 = "0.6.1"
 ecb = "=0.1.2"
 elliptic-curve = { version = "0.13.4", features = ["alloc", "arithmetic", "ecdh", "std", "pem"] }

Cargo.toml

@@ -60,7 +60,7 @@ winres.workspace = true
 
 [dependencies]
 deno_ast = { workspace = true, features = ["bundler", "cjs", "codegen", "proposal", "react", "sourcemap", "transforms", "typescript", "view", "visit"] }
-deno_cache_dir = "=0.6.1"
+deno_cache_dir = { workspace = true }
 deno_config = "=0.9.2"
 deno_core = { workspace = true, features = ["include_js_files_for_snapshotting"] }
 deno_doc = { version = "=0.103.0", features = ["html"] }

cli/Cargo.toml

@@ -100,20 +100,16 @@ impl File {
   }
 }
 
-/// Simple struct implementing in-process caching to prevent multiple
-/// fs reads/net fetches for same file.
 #[derive(Debug, Clone, Default)]
-struct FileCache(Arc<Mutex<HashMap<ModuleSpecifier, File>>>);
+struct MemoryFiles(Arc<Mutex<HashMap<ModuleSpecifier, File>>>);
 
-impl FileCache {
+impl MemoryFiles {
   pub fn get(&self, specifier: &ModuleSpecifier) -> Option<File> {
-    let cache = self.0.lock();
-    cache.get(specifier).cloned()
+    self.0.lock().get(specifier).cloned()
   }
 
   pub fn insert(&self, specifier: ModuleSpecifier, file: File) -> Option<File> {
-    let mut cache = self.0.lock();
-    cache.insert(specifier, file)
+    self.0.lock().insert(specifier, file)
   }
 }
 
@@ -157,7 +153,7 @@ pub struct FetchOptions<'a> {
 pub struct FileFetcher {
   auth_tokens: AuthTokens,
   allow_remote: bool,
-  cache: FileCache,
+  memory_files: MemoryFiles,
   cache_setting: CacheSetting,
   http_cache: Arc<dyn HttpCache>,
   http_client: Arc<HttpClient>,
@@ -178,7 +174,7 @@ impl FileFetcher {
     Self {
       auth_tokens: AuthTokens::new(env::var("DENO_AUTH_TOKENS").ok()),
       allow_remote,
-      cache: Default::default(),
+      memory_files: Default::default(),
       cache_setting,
       http_cache,
       http_client,
@@ -498,7 +494,7 @@ impl FileFetcher {
     debug!("FileFetcher::fetch() - specifier: {}", specifier);
     let scheme = get_validated_scheme(specifier)?;
     options.permissions.check_specifier(specifier)?;
-    if let Some(file) = self.cache.get(specifier) {
+    if let Some(file) = self.memory_files.get(specifier) {
       Ok(file)
     } else if scheme == "file" {
       // we do not in memory cache files, as this would prevent files on the
@@ -514,27 +510,23 @@ impl FileFetcher {
         format!("A remote specifier was requested: \"{specifier}\", but --no-remote is specified."),
       ))
     } else {
-      let result = self
+      self
         .fetch_remote(
           specifier,
           options.permissions,
           10,
           options.maybe_accept.map(String::from),
           options.maybe_cache_setting.unwrap_or(&self.cache_setting),
         )
-        .await;
-      if let Ok(file) = &result {
-        self.cache.insert(specifier.clone(), file.clone());
-      }
-      result
+        .await
     }
   }
 
   /// A synchronous way to retrieve a source file, where if the file has already
   /// been cached in memory it will be returned, otherwise for local files will
   /// be read from disk.
   pub fn get_source(&self, specifier: &ModuleSpecifier) -> Option<File> {
-    let maybe_file = self.cache.get(specifier);
+    let maybe_file = self.memory_files.get(specifier);
     if maybe_file.is_none() {
       let is_local = specifier.scheme() == "file";
       if is_local {
@@ -548,9 +540,9 @@ impl FileFetcher {
     }
   }
 
-  /// Insert a temporary module into the in memory cache for the file fetcher.
-  pub fn insert_cached(&self, file: File) -> Option<File> {
-    self.cache.insert(file.specifier.clone(), file)
+  /// Insert a temporary module for the file fetcher.
+  pub fn insert_memory_files(&self, file: File) -> Option<File> {
+    self.memory_files.insert(file.specifier.clone(), file)
   }
 }
 
@@ -826,7 +818,7 @@ mod tests {
         "application/javascript".to_string(),
       )])),
     };
-    file_fetcher.insert_cached(file.clone());
+    file_fetcher.insert_memory_files(file.clone());
 
     let result = file_fetcher
       .fetch(&specifier, PermissionsContainer::allow_all())
@@ -836,30 +828,6 @@ mod tests {
     assert_eq!(result_file, file);
   }
 
-  #[tokio::test]
-  async fn test_get_source() {
-    let _http_server_guard = test_util::http_server();
-    let (file_fetcher, _) = setup(CacheSetting::Use, None);
-    let specifier =
-      resolve_url("http://localhost:4548/subdir/redirects/redirect1.js")
-        .unwrap();
-
-    let result = file_fetcher
-      .fetch(&specifier, PermissionsContainer::allow_all())
-      .await;
-    assert!(result.is_ok());
-
-    let maybe_file = file_fetcher.get_source(&specifier);
-    assert!(maybe_file.is_some());
-    let file = maybe_file.unwrap().into_text_decoded().unwrap();
-    assert_eq!(file.source.as_ref(), "export const redirect = 1;\n");
-    assert_eq!(
-      file.specifier,
-      resolve_url("http://localhost:4545/subdir/redirects/redirect1.js")
-        .unwrap()
-    );
-  }
-
   #[tokio::test]
   async fn test_fetch_data_url() {
     let (file_fetcher, _) = setup(CacheSetting::Use, None);

cli/file_fetcher.rs

@@ -24,6 +24,7 @@ required-features = ["run"]
 bytes.workspace = true
 deno_ast.workspace = true
 deno_bench_util.workspace = true
+deno_cache_dir = { workspace = true }
 deno_core = { workspace = true, features = ["include_js_files_for_snapshotting", "unsafe_use_unprotected_platform"] }
 deno_fetch.workspace = true
 deno_lockfile.workspace = true

cli/tests/Cargo.toml

@@ -1,7 +1,9 @@
 // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
 
+use deno_core::serde_json::Value;
 use deno_lockfile::Lockfile;
 use test_util as util;
+use url::Url;
 use util::env_vars_for_jsr_tests;
 use util::TestContextBuilder;
 
@@ -105,3 +107,77 @@ console.log(version);"#,
     .run()
     .assert_matches_text("0.1.0\n");
 }
+
+#[test]
+fn reload_info_not_found_cache_but_exists_remote() {
+  fn remove_version(registry_json: &mut Value, version: &str) {
+    registry_json
+      .as_object_mut()
+      .unwrap()
+      .get_mut("versions")
+      .unwrap()
+      .as_object_mut()
+      .unwrap()
+      .remove(version);
+  }
+
+  fn remove_version_for_package(
+    deno_dir: &util::TempDir,
+    package: &str,
+    version: &str,
+  ) {
+    let specifier =
+      Url::parse(&format!("http://127.0.0.1:4250/{}/meta.json", package))
+        .unwrap();
+    let registry_json_path = deno_dir
+      .path()
+      .join("deps")
+      .join(deno_cache_dir::url_to_filename(&specifier).unwrap());
+    let mut registry_json = registry_json_path.read_json_value();
+    remove_version(&mut registry_json, version);
+    registry_json_path.write_json(&registry_json);
+  }
+
+  // This tests that when a local machine doesn't have a version
+  // specified in a dependency that exists in the npm registry
+  let test_context = TestContextBuilder::for_jsr().use_temp_cwd().build();
+  let deno_dir = test_context.deno_dir();
+  let temp_dir = test_context.temp_dir();
+  temp_dir.write(
+    "main.ts",
+    "import { add } from 'jsr:@denotest/add@1'; console.log(add(1, 2));",
+  );
+
+  // cache successfully to the deno_dir
+  let output = test_context.new_command().args("cache main.ts").run();
+  output.assert_matches_text(concat!(
+    "Download http://127.0.0.1:4250/@denotest/add/meta.json\n",
+    "Download http://127.0.0.1:4250/@denotest/add/1.0.0_meta.json\n",
+    "Download http://127.0.0.1:4250/@denotest/add/1.0.0/mod.ts\n",
+  ));
+
+  // modify the package information in the cache to remove the latest version
+  remove_version_for_package(deno_dir, "@denotest/add", "1.0.0");
+
+  // should error when `--cache-only` is used now because the version is not in the cache
+  let output = test_context
+    .new_command()
+    .args("run --cached-only main.ts")
+    .run();
+  output.assert_exit_code(1);
+  output.assert_matches_text("error: Failed to resolve version constraint. Try running again without --cached-only
+    at file:///[WILDCARD]main.ts:1:21
+");
+
+  // now try running without it, it should download the package now
+  test_context
+    .new_command()
+    .args("run main.ts")
+    .run()
+    .assert_matches_text(concat!(
+      "Download http://127.0.0.1:4250/@denotest/add/meta.json\n",
+      "Download http://127.0.0.1:4250/@denotest/add/1.0.0_meta.json\n",
+      "3\n",
+    ))
+    .assert_exit_code(0);
+}

cli/tests/integration/jsr_tests.rs

@@ -1,4 +1,5 @@
 Download http://127.0.0.1:4250/@denotest/deps/meta.json
+Download http://127.0.0.1:4250/@denotest/deps/meta.json
 error: Could not find constraint in the list of versions: @denotest/[email protected]
   Specifier: jsr:@denotest/[email protected]/mod.ts
     at file:///[WILDCARD]/version_not_found/main.ts:1:19

cli/tests/testdata/jsr/version_not_found/main.out

@@ -91,7 +91,7 @@ pub async fn run_from_stdin(flags: Flags) -> Result<i32, AnyError> {
   std::io::stdin().read_to_end(&mut source)?;
   // Save a fake file into file fetcher cache
   // to allow module access by TS compiler
-  file_fetcher.insert_cached(File {
+  file_fetcher.insert_memory_files(File {
     specifier: main_module.clone(),
     maybe_headers: None,
     source: source.into(),
@@ -174,7 +174,7 @@ pub async fn eval_command(
 
   // Save a fake file into file fetcher cache
   // to allow module access by TS compiler.
-  file_fetcher.insert_cached(File {
+  file_fetcher.insert_memory_files(File {
     specifier: main_module.clone(),
     maybe_headers: None,
     source: source_code.into_bytes().into(),

cli/tools/run/mod.rs

@@ -873,7 +873,7 @@ pub async fn check_specifiers(
       .collect();
 
     for file in inline_files {
-      file_fetcher.insert_cached(file);
+      file_fetcher.insert_memory_files(file);
     }
 
     module_load_preparer