fix: prevent cache db errors when deno_dir not exists (#23168)

Closes #20202

cli/cache/cache_db.rs

@@ -10,6 +10,7 @@ use deno_runtime::deno_webstorage::rusqlite::OptionalExtension;
 use deno_runtime::deno_webstorage::rusqlite::Params;
 use once_cell::sync::OnceCell;
 use std::io::IsTerminal;
+use std::path::Path;
 use std::path::PathBuf;
 use std::sync::Arc;
 
@@ -178,7 +179,7 @@ impl CacheDB {
   /// Open the connection in memory or on disk.
   fn actually_open_connection(
     &self,
-    path: &Option<PathBuf>,
+    path: Option<&Path>,
   ) -> Result<Connection, rusqlite::Error> {
     match path {
       // This should never fail unless something is very wrong
@@ -224,7 +225,7 @@ impl CacheDB {
   /// Open and initialize a connection.
   fn open_connection_and_init(
     &self,
-    path: &Option<PathBuf>,
+    path: Option<&Path>,
   ) -> Result<Connection, AnyError> {
     let conn = self.actually_open_connection(path)?;
     Self::initialize_connection(self.config, &conn, self.version)?;
@@ -234,83 +235,9 @@ impl CacheDB {
   /// This function represents the policy for dealing with corrupted cache files. We try fairly aggressively
   /// to repair the situation, and if we can't, we prefer to log noisily and continue with in-memory caches.
   fn open_connection(&self) -> Result<ConnectionState, AnyError> {
-    // Success on first try? We hope that this is the case.
-    let err = match self.open_connection_and_init(&self.path) {
-      Ok(conn) => return Ok(ConnectionState::Connected(conn)),
-      Err(err) => err,
-    };
-
-    if self.path.is_none() {
-      // If an in-memory DB fails, that's game over
-      log::error!("Failed to initialize in-memory cache database.");
-      return Err(err);
-    }
-
-    let path = self.path.as_ref().unwrap();
-
-    // There are rare times in the tests when we can't initialize a cache DB the first time, but it succeeds the second time, so
-    // we don't log these at a debug level.
-    log::trace!(
-      "Could not initialize cache database '{}', retrying... ({err:?})",
-      path.to_string_lossy(),
-    );
-
-    // Try a second time
-    let err = match self.open_connection_and_init(&self.path) {
-      Ok(conn) => return Ok(ConnectionState::Connected(conn)),
-      Err(err) => err,
-    };
-
-    // Failed, try deleting it
-    let is_tty = std::io::stderr().is_terminal();
-    log::log!(
-      if is_tty { log::Level::Warn } else { log::Level::Trace },
-      "Could not initialize cache database '{}', deleting and retrying... ({err:?})",
-      path.to_string_lossy()
-    );
-    if std::fs::remove_file(path).is_ok() {
-      // Try a third time if we successfully deleted it
-      let res = self.open_connection_and_init(&self.path);
-      if let Ok(conn) = res {
-        return Ok(ConnectionState::Connected(conn));
-      };
-    }
-
-    match self.config.on_failure {
-      CacheFailure::InMemory => {
-        log::log!(
-          if is_tty {
-            log::Level::Error
-          } else {
-            log::Level::Trace
-          },
-          "Failed to open cache file '{}', opening in-memory cache.",
-          path.to_string_lossy()
-        );
-        Ok(ConnectionState::Connected(
-          self.open_connection_and_init(&None)?,
-        ))
-      }
-      CacheFailure::Blackhole => {
-        log::log!(
-          if is_tty {
-            log::Level::Error
-          } else {
-            log::Level::Trace
-          },
-          "Failed to open cache file '{}', performance may be degraded.",
-          path.to_string_lossy()
-        );
-        Ok(ConnectionState::Blackhole)
-      }
-      CacheFailure::Error => {
-        log::error!(
-          "Failed to open cache file '{}', expect further errors.",
-          path.to_string_lossy()
-        );
-        Err(err)
-      }
-    }
+    open_connection(self.config, self.path.as_deref(), |maybe_path| {
+      self.open_connection_and_init(maybe_path)
+    })
   }
 
   fn initialize<'a>(
@@ -397,8 +324,105 @@ impl CacheDB {
   }
 }
 
+/// This function represents the policy for dealing with corrupted cache files. We try fairly aggressively
+/// to repair the situation, and if we can't, we prefer to log noisily and continue with in-memory caches.
+fn open_connection(
+  config: &CacheDBConfiguration,
+  path: Option<&Path>,
+  open_connection_and_init: impl Fn(Option<&Path>) -> Result<Connection, AnyError>,
+) -> Result<ConnectionState, AnyError> {
+  // Success on first try? We hope that this is the case.
+  let err = match open_connection_and_init(path) {
+    Ok(conn) => return Ok(ConnectionState::Connected(conn)),
+    Err(err) => err,
+  };
+
+  let Some(path) = path.as_ref() else {
+    // If an in-memory DB fails, that's game over
+    log::error!("Failed to initialize in-memory cache database.");
+    return Err(err);
+  };
+
+  // ensure the parent directory exists
+  if let Some(parent) = path.parent() {
+    match std::fs::create_dir_all(parent) {
+      Ok(_) => {
+        log::debug!("Created parent directory for cache db.");
+      }
+      Err(err) => {
+        log::debug!("Failed creating the cache db parent dir: {:#}", err);
+      }
+    }
+  }
+
+  // There are rare times in the tests when we can't initialize a cache DB the first time, but it succeeds the second time, so
+  // we don't log these at a debug level.
+  log::trace!(
+    "Could not initialize cache database '{}', retrying... ({err:?})",
+    path.to_string_lossy(),
+  );
+
+  // Try a second time
+  let err = match open_connection_and_init(Some(path)) {
+    Ok(conn) => return Ok(ConnectionState::Connected(conn)),
+    Err(err) => err,
+  };
+
+  // Failed, try deleting it
+  let is_tty = std::io::stderr().is_terminal();
+  log::log!(
+      if is_tty { log::Level::Warn } else { log::Level::Trace },
+      "Could not initialize cache database '{}', deleting and retrying... ({err:?})",
+      path.to_string_lossy()
+    );
+  if std::fs::remove_file(path).is_ok() {
+    // Try a third time if we successfully deleted it
+    let res = open_connection_and_init(Some(path));
+    if let Ok(conn) = res {
+      return Ok(ConnectionState::Connected(conn));
+    };
+  }
+
+  match config.on_failure {
+    CacheFailure::InMemory => {
+      log::log!(
+        if is_tty {
+          log::Level::Error
+        } else {
+          log::Level::Trace
+        },
+        "Failed to open cache file '{}', opening in-memory cache.",
+        path.to_string_lossy()
+      );
+      Ok(ConnectionState::Connected(open_connection_and_init(None)?))
+    }
+    CacheFailure::Blackhole => {
+      log::log!(
+        if is_tty {
+          log::Level::Error
+        } else {
+          log::Level::Trace
+        },
+        "Failed to open cache file '{}', performance may be degraded.",
+        path.to_string_lossy()
+      );
+      Ok(ConnectionState::Blackhole)
+    }
+    CacheFailure::Error => {
+      log::error!(
+        "Failed to open cache file '{}', expect further errors.",
+        path.to_string_lossy()
+      );
+      Err(err)
+    }
+  }
+}
+
 #[cfg(test)]
 mod tests {
+  use deno_core::anyhow::anyhow;
+  use test_util::TempDir;
+
   use super::*;
 
   static TEST_DB: CacheDBConfiguration = CacheDBConfiguration {
@@ -409,15 +433,15 @@ mod tests {
   };
 
   static TEST_DB_BLACKHOLE: CacheDBConfiguration = CacheDBConfiguration {
-    table_initializer: "create table if not exists test(value TEXT);",
-    on_version_change: "delete from test;",
+    table_initializer: "syntax error", // intentially cause an error
+    on_version_change: "",
     preheat_queries: &[],
     on_failure: CacheFailure::Blackhole,
   };
 
   static TEST_DB_ERROR: CacheDBConfiguration = CacheDBConfiguration {
-    table_initializer: "create table if not exists test(value TEXT);",
-    on_version_change: "delete from test;",
+    table_initializer: "syntax error", // intentionally cause an error
+    on_version_change: "",
     preheat_queries: &[],
     on_failure: CacheFailure::Error,
   };
@@ -429,8 +453,6 @@ mod tests {
     on_failure: CacheFailure::InMemory,
   };
 
-  static FAILURE_PATH: &str = "/tmp/this/doesnt/exist/so/will/always/fail";
-
   #[tokio::test]
   async fn simple_database() {
     let db = CacheDB::in_memory(&TEST_DB, "1.0");
@@ -443,7 +465,7 @@ mod tests {
         Ok(row.get::<_, String>(0).unwrap())
       })
       .unwrap();
-    assert_eq!(Some("1".into()), res);
+    assert_eq!(res, Some("1".into()));
   }
 
   #[tokio::test]
@@ -455,22 +477,23 @@ mod tests {
 
   #[tokio::test]
   async fn failure_mode_in_memory() {
-    let db = CacheDB::from_path(&TEST_DB, FAILURE_PATH.into(), "1.0");
-    db.ensure_connected()
-      .expect("Should have created a database");
-
-    db.execute("insert into test values (?1)", [1]).unwrap();
-    let res = db
-      .query_row("select * from test", [], |row| {
-        Ok(row.get::<_, String>(0).unwrap())
-      })
-      .unwrap();
-    assert_eq!(Some("1".into()), res);
+    let temp_dir = TempDir::new();
+    let path = temp_dir.path().join("data").to_path_buf();
+    let state = open_connection(&TEST_DB, Some(path.as_path()), |maybe_path| {
+      match maybe_path {
+        Some(_) => Err(anyhow!("fail")),
+        None => Ok(Connection::open_in_memory().unwrap()),
+      }
+    })
+    .unwrap();
+    assert!(matches!(state, ConnectionState::Connected(_)));
   }
 
   #[tokio::test]
   async fn failure_mode_blackhole() {
-    let db = CacheDB::from_path(&TEST_DB_BLACKHOLE, FAILURE_PATH.into(), "1.0");
+    let temp_dir = TempDir::new();
+    let path = temp_dir.path().join("data");
+    let db = CacheDB::from_path(&TEST_DB_BLACKHOLE, path.to_path_buf(), "1.0");
     db.ensure_connected()
       .expect("Should have created a database");
 
@@ -480,12 +503,14 @@ mod tests {
         Ok(row.get::<_, String>(0).unwrap())
       })
       .unwrap();
-    assert_eq!(None, res);
+    assert_eq!(res, None);
   }
 
   #[tokio::test]
   async fn failure_mode_error() {
-    let db = CacheDB::from_path(&TEST_DB_ERROR, FAILURE_PATH.into(), "1.0");
+    let temp_dir = TempDir::new();
+    let path = temp_dir.path().join("data");
+    let db = CacheDB::from_path(&TEST_DB_ERROR, path.to_path_buf(), "1.0");
     db.ensure_connected().expect_err("Should have failed");
 
     db.execute("insert into test values (?1)", [1])

tests/specs/fmt/no_error_deno_dir_not_exists/__test__.jsonc

@@ -0,0 +1,8 @@
+{
+  "tempDir": true,
+  "envs": {
+    "DENO_DIR": "$DENO_DIR/not_exists"
+  },
+  "args": "fmt --log-level=debug main.ts",
+  "output": "fmt.out"
+}

tests/specs/fmt/no_error_deno_dir_not_exists/fmt.out

@@ -0,0 +1,2 @@
+[WILDCARD]Created parent directory for cache db.[WILDCARD]
+Checked 1 file

tests/specs/fmt/no_error_deno_dir_not_exists/main.ts

@@ -0,0 +1 @@
+console.log(1);

tests/specs/mod.rs

@@ -128,7 +128,9 @@ fn run_test(test: &Test, diagnostic_logger: Rc<RefCell<Vec<u8>>>) {
       context.deno_dir().path().remove_dir_all();
     }
 
-    let command = context.new_command().envs(&step.envs);
+    let command = context
+      .new_command()
+      .envs(metadata.envs.iter().chain(step.envs.iter()));
     let command = match &step.args {
       VecOrString::Vec(args) => command.args_vec(args),
       VecOrString::String(text) => command.args(text),
@@ -166,6 +168,8 @@ struct MultiTestMetaData {
   /// The base environment to use for the test.
   #[serde(default)]
   pub base: Option<String>,
+  #[serde(default)]
+  pub envs: HashMap<String, String>,
   pub steps: Vec<StepMetaData>,
 }
 
@@ -185,6 +189,7 @@ impl SingleTestMetaData {
     MultiTestMetaData {
       base: self.base,
       temp_dir: self.temp_dir,
+      envs: Default::default(),
       steps: vec![self.step],
     }
   }

tests/util/server/src/builders.rs

@@ -782,6 +782,13 @@ impl TestCommandBuilder {
     for key in &self.envs_remove {
       envs.remove(key);
     }
+
+    // update any test variables in the env value
+    for value in envs.values_mut() {
+      *value =
+        value.replace("$DENO_DIR", &self.deno_dir.path().to_string_lossy());
+    }
+
     envs
   }
 }

tests/util/server/src/fs.rs

@@ -159,8 +159,8 @@ impl PathRef {
     file.write_all(text.as_ref().as_bytes()).unwrap();
   }
 
-  pub fn write(&self, text: impl AsRef<str>) {
-    fs::write(self, text.as_ref()).unwrap();
+  pub fn write(&self, text: impl AsRef<[u8]>) {
+    fs::write(self, text).unwrap();
   }
 
   pub fn write_json<TValue: Serialize>(&self, value: &TValue) {
@@ -461,7 +461,7 @@ impl TempDir {
     self.target_path().join(from).rename(to)
   }
 
-  pub fn write(&self, path: impl AsRef<Path>, text: impl AsRef<str>) {
+  pub fn write(&self, path: impl AsRef<Path>, text: impl AsRef<[u8]>) {
     self.target_path().join(path).write(text)
   }