Skip to content

Commit

Permalink
feat: Added warning when failing to update index cache (#15014)
Browse files Browse the repository at this point in the history
<!--
Thanks for submitting a pull request 🎉! Here are some tips for you:

* If this is your first contribution, read "Cargo Contribution Guide"
first:
  https://doc.crates.io/contrib/
* Run `cargo fmt --all` to format your code changes.
* Small commits and pull requests are always preferable and easy to
review.
* If your idea is large and needs feedback from the community, read how:
  https://doc.crates.io/contrib/process/#working-on-large-features
* Cargo takes care of compatibility. Read our design principles:
  https://doc.crates.io/contrib/design.html
* When changing help text of cargo commands, follow the steps to
generate docs:

https://github.com/rust-lang/cargo/tree/master/src/doc#building-the-man-pages
* If your PR is not finished, set it as "draft" PR or add "WIP" in its
title.
* It's ok to use the CI resources to test your PR, but please don't
abuse them.

### What does this PR try to resolve?

Explain the motivation behind this change.
A clear overview along with an in-depth explanation are helpful.

You can use `Fixes #<issue number>` to associate this PR to an existing
issue.

### How should we test and review this PR?

Demonstrate how you test this change and guide reviewers through your
PR.
With a smooth review process, a pull request usually gets reviewed
quicker.

If you don't know how to write and run your tests, please read the
guide:
https://doc.crates.io/contrib/tests

### Additional information

Other information you want to mention in this PR, such as prior arts,
future extensions, an unresolved problem, or a TODO list.
-->

### What does this PR try to resolve?

Fixes #13712

Adds a warning if there is an error updating the index cache.
It also attempts to avoid warning spam as requested in [this
comment](#13712 (comment))

Below is an example output

```
    Updating crates.io index
warning: failed to write cache, path: /home/ross/.cargo/registry/index/index.crates.io-1949cf8c6b5b557f/.cache/ba/se/base64, error: Permission denied (os error 13)
   Compiling serde v1.0.217
   Compiling r-efi v5.2.0
   Compiling base64 v0.22.1
   Compiling cargo-13712 v0.1.0 (/home/ross/projects/cargo-13712)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.20s

```

### How should we test and review this PR?

I tested this on my machine by manually changing the owner/permission of
the index files
```sh
sudo chown root ~/.cargo/registry/index/.../.cache/r-
sudo chmod 700 ~/.cargo/registry/index/.../.cache/r-

# in a project that uses the `r-efi` crate
cargo build
```

I wasn't quiet sure about the best way to add a test to the testsuite
for this. 😅
If there is good way to create a test for this lmk
  • Loading branch information
weihanglo authored Jan 10, 2025
2 parents eb69b44 + 272d07a commit c518f22
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 8 deletions.
34 changes: 27 additions & 7 deletions src/cargo/sources/registry/index/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -226,14 +227,21 @@ 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<bool>,
}

impl<'gctx> CacheManager<'gctx> {
/// Creates a new instance of the on-disk index cache manager.
///
/// `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.
Expand All @@ -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);
Expand Down
74 changes: 73 additions & 1 deletion tests/testsuite/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -3097,6 +3097,78 @@ fn readonly_registry_still_works() {
}
}

#[cargo_test(ignore_windows = "On Windows setting file attributes is a bit complicated")]
fn unaccessible_registry_cache_still_works() {
Package::new("foo", "0.1.0").publish();
Package::new("fo2", "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'
fo2 = '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/fo[..], [ERROR] Permission denied (os error 13)
[COMPILING] fo[..] v0.1.0
[COMPILING] fo[..] 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) {
#[cfg(not(windows))]
{
use std::os::unix::fs::PermissionsExt;
let mut perms = t!(path.metadata()).permissions();
perms.set_mode(permissions);
t!(fs::set_permissions(path, perms));
}

#[cfg(windows)]
panic!("This test is not supported on windows. See the reason in the #[cargo_test] macro");
}

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();
Expand Down

0 comments on commit c518f22

Please sign in to comment.