cargo vendor modifies the package cache without locking

[Amanieu]

We are getting occasional, but infrequent, failures in our CI which uses cargo vendor. Looking into it, it seems that this could be due to alexcrichton/cargo-vendor#131, where cargo vendor will delete downloaded crates in the package cache, even though those crates could be concurrently used by other build processes.

This is done without holding the package cache lock, but even if it did I'm not sure this is correct since other cargo commands will read from the package cache (e.g. cargo build will read the code from extracted crates) without holding the package cache lock.

[weihanglo]

I think your observation is correct. This line does delete sources from cargo glocal package source cache.

cargo/src/cargo/ops/vendor.rs

Line 138 in 928b956

drop(fs::remove_dir_all(pkg.manifest_path().parent().unwrap()));

This is done without holding the package cache lock, but even if it did I'm not sure this is correct since other cargo commands will read from the package cache (e.g. cargo build will read the code from extracted crates) without holding the package cache lock.

I think the suggested approach is pretty much doable, and my impression is that during the dependency resolution cargo build also need to acquire package-cache lock. May be as easy as putting a line above here to acquire the package cache lock. Just the heavy-hammer approach for removing-then-redownloading is still a bit uncomfortable to me in terms of performance.

---

triage:

@rustbot label +C-bug +Command-vendor +S-accepted +E-easy

[0xPoe]

@rustbot claim

[Amanieu]

I don't think cargo build needs to acquire the package cache lock because under normal circumstances, the package cache is append-only: extracted crates are never modified. Only access to the index (which is modified by updates) requires taking the package cache lock.

Also we specifically don't want to acquire the package lock for cargo build since that woud prevent parallel builds from multiple cargo instances.

I think the best way forward might be to just revert alexcrichton/cargo-vendor#131.

[weihanglo]

cargo build is already acquiring the package cache lock if I rememeber correctly. It isn't? 🤔

[Amanieu]

Only for dependency resolution and downloading crates, not during the actual build itself. However it still needs to read the crate source code from the package cache during the build.

[weihanglo]

However it still needs to read the crate source code from the package cache during the build.

That's what I was saying. cargo build gets the lock for reading.

I think the best way forward might be to just revert alexcrichton/cargo-vendor#131.

The bad news is that ~/.cargo/registry/src is not readonly at this moment. If we want to make sure the integrity a bit, we might be better starting over from unpacking. It also sounds safer to me to get the advisory lock every time when reading source code from package cache. Otherwise we'll still have a chance that other process gets the lock and modify stuff during our reading.

[Amanieu]

Currently we only have a single way to acquire the package cache lock: acquire_package_cache_lock which acquires an exclusive lock. We could extend this to support both shared and exclusive locks, and acquire a shared lock whenever code from the package cache is read.

[Amanieu]

This turns out to be more complicated than I expected: the package cache lock is a recursive lock and OSes don't like it when you acquire both a read and write lock on the same file.

I still think the best way forward is to revert alexcrichton/cargo-vendor#131: we should never be modifying code for extracted crates.