From b0c1bd82a85ddb54ffe717a2c158c33c0be99fe8 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 1 Apr 2024 18:58:52 -0400 Subject: [PATCH] fix: prevent cache db errors when deno_dir not exists (#23168) Closes #20202 --- cli/cache/cache_db.rs | 225 ++++++++++-------- .../__test__.jsonc | 8 + .../fmt/no_error_deno_dir_not_exists/fmt.out | 2 + .../fmt/no_error_deno_dir_not_exists/main.ts | 1 + tests/specs/mod.rs | 7 +- tests/util/server/src/builders.rs | 7 + tests/util/server/src/fs.rs | 6 +- 7 files changed, 152 insertions(+), 104 deletions(-) create mode 100644 tests/specs/fmt/no_error_deno_dir_not_exists/__test__.jsonc create mode 100644 tests/specs/fmt/no_error_deno_dir_not_exists/fmt.out create mode 100644 tests/specs/fmt/no_error_deno_dir_not_exists/main.ts diff --git a/cli/cache/cache_db.rs b/cli/cache/cache_db.rs index d6385569af..0613d52c66 100644 --- a/cli/cache/cache_db.rs +++ b/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, + path: Option<&Path>, ) -> Result { 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, + path: Option<&Path>, ) -> Result { 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 { - // 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, +) -> Result { + // 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]) diff --git a/tests/specs/fmt/no_error_deno_dir_not_exists/__test__.jsonc b/tests/specs/fmt/no_error_deno_dir_not_exists/__test__.jsonc new file mode 100644 index 0000000000..4e4f67477f --- /dev/null +++ b/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" +} diff --git a/tests/specs/fmt/no_error_deno_dir_not_exists/fmt.out b/tests/specs/fmt/no_error_deno_dir_not_exists/fmt.out new file mode 100644 index 0000000000..930bcb5c64 --- /dev/null +++ b/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 diff --git a/tests/specs/fmt/no_error_deno_dir_not_exists/main.ts b/tests/specs/fmt/no_error_deno_dir_not_exists/main.ts new file mode 100644 index 0000000000..296d5492b0 --- /dev/null +++ b/tests/specs/fmt/no_error_deno_dir_not_exists/main.ts @@ -0,0 +1 @@ +console.log(1); diff --git a/tests/specs/mod.rs b/tests/specs/mod.rs index 3eb8bb3866..17263b8b9c 100644 --- a/tests/specs/mod.rs +++ b/tests/specs/mod.rs @@ -128,7 +128,9 @@ fn run_test(test: &Test, diagnostic_logger: Rc>>) { 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, + #[serde(default)] + pub envs: HashMap, pub steps: Vec, } @@ -185,6 +189,7 @@ impl SingleTestMetaData { MultiTestMetaData { base: self.base, temp_dir: self.temp_dir, + envs: Default::default(), steps: vec![self.step], } } diff --git a/tests/util/server/src/builders.rs b/tests/util/server/src/builders.rs index e1d351da8c..6a57548a3a 100644 --- a/tests/util/server/src/builders.rs +++ b/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 } } diff --git a/tests/util/server/src/fs.rs b/tests/util/server/src/fs.rs index 8955dc30ee..b9ae81b492 100644 --- a/tests/util/server/src/fs.rs +++ b/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) { - fs::write(self, text.as_ref()).unwrap(); + pub fn write(&self, text: impl AsRef<[u8]>) { + fs::write(self, text).unwrap(); } pub fn write_json(&self, value: &TValue) { @@ -461,7 +461,7 @@ impl TempDir { self.target_path().join(from).rename(to) } - pub fn write(&self, path: impl AsRef, text: impl AsRef) { + pub fn write(&self, path: impl AsRef, text: impl AsRef<[u8]>) { self.target_path().join(path).write(text) }