From 08faceec3281713ba1a63b949c973df3dfcb5577 Mon Sep 17 00:00:00 2001 From: ranger-ross Date: Sat, 4 Jan 2025 12:31:15 +0900 Subject: [PATCH 1/2] feat: Added warning when failing to update index cache --- src/cargo/sources/registry/index/cache.rs | 34 ++++++++++++++++++----- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/src/cargo/sources/registry/index/cache.rs b/src/cargo/sources/registry/index/cache.rs index 5d3bb28500a..f6e7d96ba6a 100644 --- a/src/cargo/sources/registry/index/cache.rs +++ b/src/cargo/sources/registry/index/cache.rs @@ -65,6 +65,7 @@ //! [`IndexSummary::parse`]: super::IndexSummary::parse //! [`RemoteRegistry`]: crate::sources::registry::remote::RemoteRegistry +use std::cell::RefCell; use std::fs; use std::io; use std::path::PathBuf; @@ -226,6 +227,9 @@ pub struct CacheManager<'gctx> { cache_root: Filesystem, /// [`GlobalContext`] reference for convenience. gctx: &'gctx GlobalContext, + /// Keeps track of if we have sent a warning message if there was an error updating the cache. + /// The motivation is to avoid warning spam if the cache is not writable. + has_warned: RefCell, } impl<'gctx> CacheManager<'gctx> { @@ -233,7 +237,11 @@ impl<'gctx> CacheManager<'gctx> { /// /// `root` --- The root path where caches are located. pub fn new(cache_root: Filesystem, gctx: &'gctx GlobalContext) -> CacheManager<'gctx> { - CacheManager { cache_root, gctx } + CacheManager { + cache_root, + gctx, + has_warned: Default::default(), + } } /// Gets the cache associated with the key. @@ -251,16 +259,28 @@ impl<'gctx> CacheManager<'gctx> { /// Associates the value with the key. pub fn put(&self, key: &str, value: &[u8]) { let cache_path = &self.cache_path(key); - if fs::create_dir_all(cache_path.parent().unwrap()).is_ok() { - let path = Filesystem::new(cache_path.clone()); - self.gctx - .assert_package_cache_locked(CacheLockMode::DownloadExclusive, &path); - if let Err(e) = fs::write(cache_path, value) { - tracing::info!(?cache_path, "failed to write cache: {e}"); + if let Err(e) = self.put_inner(cache_path, value) { + tracing::info!(?cache_path, "failed to write cache: {e}"); + + if !*self.has_warned.borrow() { + let _ = self.gctx.shell().warn(format!( + "failed to write cache, path: {}, error: {e}", + cache_path.to_str().unwrap_or_default() + )); + *self.has_warned.borrow_mut() = true; } } } + fn put_inner(&self, cache_path: &PathBuf, value: &[u8]) -> std::io::Result<()> { + fs::create_dir_all(cache_path.parent().unwrap())?; + let path = Filesystem::new(cache_path.clone()); + self.gctx + .assert_package_cache_locked(CacheLockMode::DownloadExclusive, &path); + fs::write(cache_path, value)?; + Ok(()) + } + /// Invalidates the cache associated with the key. pub fn invalidate(&self, key: &str) { let cache_path = &self.cache_path(key); From 92ed0f138d5a76b72d167b7cc0c3ef615d3966bc Mon Sep 17 00:00:00 2001 From: ranger-ross Date: Wed, 8 Jan 2025 23:31:30 +0900 Subject: [PATCH 2/2] test: Added test to verify registry cache write error emit warnings --- tests/testsuite/registry.rs | 66 ++++++++++++++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index 8498713701c..3f79a327137 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -2,7 +2,7 @@ use std::fmt::Write; use std::fs::{self, File}; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::sync::Arc; use std::sync::Mutex; @@ -3097,6 +3097,70 @@ fn readonly_registry_still_works() { } } +#[cargo_test] +#[cfg(not(windows))] +fn unaccessible_registry_cache_still_works() { + Package::new("foo", "0.1.0").publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "a" + version = "0.5.0" + edition = "2015" + authors = [] + + [dependencies] + foo = '0.1.0' + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("generate-lockfile").run(); + p.cargo("fetch --locked").run(); + + let cache_path = inner_dir(&paths::cargo_home().join("registry/index")).join(".cache"); + let f_cache_path = cache_path.join("3/f"); + + // Remove the permissions from the cache path that contains the "foo" crate + set_permissions(&f_cache_path, 0o000); + + // Now run a build and make sure we properly build and warn the user + p.cargo("build") + .with_stderr_data(str![[r#" +[WARNING] failed to write cache, path: [ROOT]/home/.cargo/registry/index/-[HASH]/.cache/3/f/foo, [ERROR] Permission denied (os error 13) +[COMPILING] foo v0.1.0 +[COMPILING] a v0.5.0 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); + // make sure we add the permissions to the files afterwards so "cargo clean" can remove them (#6934) + set_permissions(&f_cache_path, 0o777); + + fn set_permissions(path: &Path, permissions: u32) { + use std::os::unix::fs::PermissionsExt; + let mut perms = t!(path.metadata()).permissions(); + perms.set_mode(permissions); + t!(fs::set_permissions(path, perms)); + } + + fn inner_dir(path: &Path) -> PathBuf { + for entry in t!(path.read_dir()) { + let path = t!(entry).path(); + + if path.is_dir() { + return path; + } + } + + panic!("could not find inner directory of {path:?}"); + } +} + #[cargo_test] fn registry_index_rejected_http() { let _server = setup_http();