Skip to content

feat: Added warning when failing to update index cache #15014

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 27 additions & 7 deletions src/cargo/sources/registry/index/cache.rs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a similar test, maybe we can take reuse that test to assert their stderr?

fn readonly_registry_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();
chmod_readonly(&paths::home(), true);
p.cargo("check").run();
// make sure we un-readonly the files afterwards so "cargo clean" can remove them (#6934)
chmod_readonly(&paths::home(), false);
fn chmod_readonly(path: &Path, readonly: bool) {
for entry in t!(path.read_dir()) {
let entry = t!(entry);
let path = entry.path();
if t!(entry.file_type()).is_dir() {
chmod_readonly(&path, readonly);
} else {
set_readonly(&path, readonly);
}
}
set_readonly(path, readonly);
}
fn set_readonly(path: &Path, readonly: bool) {
let mut perms = t!(path.metadata()).permissions();
perms.set_readonly(readonly);
t!(fs::set_permissions(path, perms));
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, I was able to use this to create a test based off of this in 9f2d0a3

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shameless copied from #15026 (comment).

We generally ask that PRs are split into

  • One commit that adds the test, showing the currently broken behavior (ie it passes)
  • The fix commit that updates the test to pass with the change

This makes the diff between the two commits show the change in behavior

Mind doing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I re-ordered the commits so that the test was added before the fix.
Let me know if I did something incorrectly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original idea is that both commits should pass CI. And the second commit contains changes in both the logic fix and the test, so the test change could reflect how the behavior has changed with the fix. See this PR as an example #15036. Anyway it doesn't really matter, and thank you!

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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add more than one dependency, so we know it really emits only once?

Copy link
Contributor Author

@ranger-ross ranger-ross Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was testing with this and notice the dependencies would get compiled in different orders causing .with_stderr_data() to fail sometimes.

But I think this can be worked around by using the [..] to ignore the names since they are not the point of this test.

    Package::new("foo", "0.1.0").publish();
    Package::new("fo2", "0.1.0").publish();

// ...

        .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

"#]])

I think something like this should.
But let me know if you are aware of a smarter way to do this. 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have the unordered method to relax the snapshot comparsion a bit, though your approach is pretty great!

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
Loading