From edb35e8b9449e97ef819d514643b46b8e1875593 Mon Sep 17 00:00:00 2001 From: Graham Christensen Date: Thu, 29 Aug 2024 16:37:49 -0400 Subject: [PATCH 1/5] Convert the Io error type to have a string of context --- magic-nix-cache/src/api.rs | 3 ++- magic-nix-cache/src/error.rs | 4 ++-- magic-nix-cache/src/util.rs | 14 ++++++++++++-- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/magic-nix-cache/src/api.rs b/magic-nix-cache/src/api.rs index 7aa6c63..104d7f1 100644 --- a/magic-nix-cache/src/api.rs +++ b/magic-nix-cache/src/api.rs @@ -112,7 +112,8 @@ async fn workflow_finish( // NOTE(cole-h): see `init_logging` if let Some(logfile) = &state.logfile { - let logfile_contents = std::fs::read_to_string(logfile)?; + let logfile_contents = std::fs::read_to_string(logfile) + .map_err(|e| crate::error::Error::Io(e, format!("Reading {}", logfile.display())))?; println!("Every log line throughout the lifetime of the program:"); println!("\n{logfile_contents}\n"); } diff --git a/magic-nix-cache/src/error.rs b/magic-nix-cache/src/error.rs index ec1b8d3..d975616 100644 --- a/magic-nix-cache/src/error.rs +++ b/magic-nix-cache/src/error.rs @@ -19,8 +19,8 @@ pub enum Error { #[error("Bad Request")] BadRequest, - #[error("I/O error: {0}")] - Io(#[from] std::io::Error), + #[error("I/O error: {0}. Context: {1}")] + Io(std::io::Error, String), #[error("GHA cache is disabled")] GHADisabled, diff --git a/magic-nix-cache/src/util.rs b/magic-nix-cache/src/util.rs index 2c7e759..db88ffb 100644 --- a/magic-nix-cache/src/util.rs +++ b/magic-nix-cache/src/util.rs @@ -11,9 +11,19 @@ use crate::error::Result; pub async fn get_store_paths(store: &NixStore) -> Result> { // FIXME: use the Nix API. let store_dir = store.store_dir(); - let mut listing = tokio::fs::read_dir(store_dir).await?; + let mut listing = tokio::fs::read_dir(store_dir).await.map_err(|e| { + crate::error::Error::Io( + e, + format!("Enumerating store paths in {}", store_dir.display()), + ) + })?; let mut paths = HashSet::new(); - while let Some(entry) = listing.next_entry().await? { + while let Some(entry) = listing.next_entry().await.map_err(|e| { + crate::error::Error::Io( + e, + format!("Reading existing store paths from {}", store_dir.display()), + ) + })? { let file_name = entry.file_name(); let file_name = Path::new(&file_name); From e52d545126407d136b3c8d42bc9a67c0c2f8c094 Mon Sep 17 00:00:00 2001 From: Graham Christensen Date: Thu, 29 Aug 2024 16:40:21 -0400 Subject: [PATCH 2/5] Convert the other Io error type to have a string of context --- gha-cache/src/api.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/gha-cache/src/api.rs b/gha-cache/src/api.rs index c1852df..be93cef 100644 --- a/gha-cache/src/api.rs +++ b/gha-cache/src/api.rs @@ -75,8 +75,8 @@ pub enum Error { info: ApiErrorInfo, }, - #[error("I/O error: {0}")] - IoError(#[from] std::io::Error), + #[error("I/O error: {0}, context: {1}")] + IoError(std::io::Error, String), #[error("Too many collisions")] TooManyCollisions, @@ -345,8 +345,9 @@ impl Api { let mut futures = Vec::new(); loop { let buf = BytesMut::with_capacity(CHUNK_SIZE); - let chunk = read_chunk_async(&mut stream, buf).await?; - + let chunk = read_chunk_async(&mut stream, buf) + .await + .map_err(|e| Error::IoError(e, "Reading a chunk during upload".to_string()))?; if chunk.is_empty() { offset += chunk.len(); break; From f0b1e6910056b9012c0f69ad945b2291c6552319 Mon Sep 17 00:00:00 2001 From: Graham Christensen Date: Thu, 29 Aug 2024 16:46:20 -0400 Subject: [PATCH 3/5] A couple more contexts in the post-build-hook --- magic-nix-cache/src/pbh.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/magic-nix-cache/src/pbh.rs b/magic-nix-cache/src/pbh.rs index b75806b..cda2ad5 100644 --- a/magic-nix-cache/src/pbh.rs +++ b/magic-nix-cache/src/pbh.rs @@ -164,9 +164,20 @@ pub async fn setup_legacy_post_build_hook( &path.display().to_string(), ]) .output() - .await?; + .await + .with_context(|| { + format!( + "Running nix to add the post-build-hook to the store from {}", + path.display() + ) + })?; if res.status.success() { - tokio::fs::remove_file(path).await?; + tokio::fs::remove_file(&path).await.with_context(|| { + format!( + "Cleaning up the temporary post-build-hook at {}", + path.display() + ) + })?; PathBuf::from(String::from_utf8_lossy(&res.stdout).trim()) } else { path From 21a9552b0bc927b6007eea7dd6aca3e9a13217e3 Mon Sep 17 00:00:00 2001 From: Cole Mickens Date: Thu, 29 Aug 2024 14:34:42 -0700 Subject: [PATCH 4/5] startup notification: create_dir_all parent dir, add context for io ops --- magic-nix-cache/src/main.rs | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/magic-nix-cache/src/main.rs b/magic-nix-cache/src/main.rs index e5fbdfa..661dd11 100644 --- a/magic-nix-cache/src/main.rs +++ b/magic-nix-cache/src/main.rs @@ -392,8 +392,33 @@ async fn main_cli() -> Result<()> { tracing::debug!("Startup notification via file at {startup_notification_file_path:?}"); - let mut notification_file = File::create(&startup_notification_file_path).await?; - notification_file.write_all(file_contents).await?; + if let Some(parent_dir) = startup_notification_file_path.parent() { + tokio::fs::create_dir_all(parent_dir) + .await + .with_context(|| { + format!( + "failed to create parent directory for startup notification file path: {}", + startup_notification_file_path.display() + ) + })?; + } + let mut notification_file = File::create(&startup_notification_file_path) + .await + .with_context(|| { + format!( + "failed to create startup notification file to path: {}", + startup_notification_file_path.display() + ) + })?; + notification_file + .write_all(file_contents) + .await + .with_context(|| { + format!( + "failed to write startup notification file to path: {}", + startup_notification_file_path.display() + ) + })?; tracing::debug!("Created startup notification file at {startup_notification_file_path:?}"); } From fa02e9ad6f0172fc48f14d816b67991f22735b28 Mon Sep 17 00:00:00 2001 From: Cole Mickens Date: Thu, 29 Aug 2024 14:42:08 -0700 Subject: [PATCH 5/5] convert unwraps to expects --- gha-cache/src/api.rs | 9 +++++++-- magic-nix-cache/src/gha.rs | 18 +++++++++++++++--- magic-nix-cache/src/main.rs | 12 ++++++++++-- 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/gha-cache/src/api.rs b/gha-cache/src/api.rs index be93cef..b7dc38f 100644 --- a/gha-cache/src/api.rs +++ b/gha-cache/src/api.rs @@ -369,7 +369,10 @@ impl Api { let url = self.construct_url(&format!("caches/{}", allocation.0 .0)); tokio::task::spawn(async move { - let permit = concurrency_limit.acquire().await.unwrap(); + let permit = concurrency_limit + .acquire() + .await + .expect("failed to acquire concurrency semaphore permit"); tracing::trace!( "Starting uploading chunk {}-{}", @@ -411,7 +414,9 @@ impl Api { future::join_all(futures) .await .into_iter() - .try_for_each(|join_result| join_result.unwrap())?; + .try_for_each(|join_result| { + join_result.expect("failed collecting a join result during parallel upload") + })?; tracing::debug!("Received all chunks for cache {:?}", allocation.0); diff --git a/magic-nix-cache/src/gha.rs b/magic-nix-cache/src/gha.rs index 00e1ba2..cd4df57 100644 --- a/magic-nix-cache/src/gha.rs +++ b/magic-nix-cache/src/gha.rs @@ -72,7 +72,9 @@ impl GhaCache { self.channel_tx .send(Request::Shutdown) .expect("Cannot send shutdown message"); - worker_result.await.unwrap() + worker_result + .await + .expect("failed to read result from gha worker") } else { Ok(()) } @@ -189,7 +191,7 @@ async fn upload_path( let narinfo = path_info_to_nar_info(store.clone(), &path_info, format!("nar/{}", nar_path)) .to_string() - .unwrap(); + .expect("failed to convert path into to nar info"); tracing::debug!("Uploading '{}'", narinfo_path); @@ -224,7 +226,17 @@ fn path_info_to_nar_info(store: Arc, path_info: &ValidPathInfo, url: S references: path_info .references .iter() - .map(|r| r.file_name().unwrap().to_str().unwrap().to_owned()) + .map(|r| { + r.file_name() + .and_then(|n| n.to_str()) + .unwrap_or_else(|| { + panic!( + "failed to convert nar_info reference to string: {}", + r.display() + ) + }) + .to_owned() + }) .collect(), system: None, deriver: None, diff --git a/magic-nix-cache/src/main.rs b/magic-nix-cache/src/main.rs index 661dd11..d73af3f 100644 --- a/magic-nix-cache/src/main.rs +++ b/magic-nix-cache/src/main.rs @@ -462,8 +462,16 @@ fn init_logging() -> Result { let filter = EnvFilter::try_from_default_env().unwrap_or_else(|_| { #[cfg(debug_assertions)] return EnvFilter::new("info") - .add_directive("magic_nix_cache=debug".parse().unwrap()) - .add_directive("gha_cache=debug".parse().unwrap()); + .add_directive( + "magic_nix_cache=debug" + .parse() + .expect("failed to parse magix_nix_cache directive"), + ) + .add_directive( + "gha_cache=debug" + .parse() + .expect("failed to parse gha_cahce directive"), + ); #[cfg(not(debug_assertions))] return EnvFilter::new("info");