fix: ensure temporary caches remain until process completion#15977
fix: ensure temporary caches remain until process completion#15977monosans wants to merge 3 commits intoastral-sh:mainfrom
Conversation
crates/uv/src/commands/tool/run.rs
Outdated
| // Unblock cache removal operations for persistent caches. If the cache is temporary | ||
| // (used when `--no-cache` was requested), we must keep it alive until the child | ||
| // process has been spawned and finished; otherwise the temporary directory will be | ||
| // deleted and the child's executable may not be present on disk. | ||
| if !cache.is_temporary() { | ||
| drop(cache); | ||
| } |
There was a problem hiding this comment.
In this case, we still to drop (or more specifically, unlock) the locked file, while keeping the temp directory around
There was a problem hiding this comment.
Why do we need to unlock it if we're the only one operating on it?
There was a problem hiding this comment.
If we keep it locked, it means that every uv run blocks any uv cache operation until it's done, this would be the strictest setting but we have talked in another thread about that we don't actually want to block parallel operations on the cache where possible.
There was a problem hiding this comment.
At least with --link-mode reflink|hardlink, I see no risk for the spawned process being affected by cache cleaning
There was a problem hiding this comment.
This is specifically with --no-cache in which a temporary dedicated cache is being used?
There was a problem hiding this comment.
So I don't think that keeping the (temporary) cache locked would block any other operations.
There was a problem hiding this comment.
If this is specifically for --no-cache, then I don't see why it's a problem to leave it locked.
| args.iter().map(|arg| arg.to_string_lossy()).join(" ") | ||
| ); | ||
|
|
||
| // Unblock cache removal operations. |
There was a problem hiding this comment.
We'll need this same change in the project run command per #15989
…15990) We're seeing reports of a regression from #15888 where `--no-cache` causes `uv run` and `uvx` to fail to spawn a command. The intent of this code was to allow destructive cache operations _after_ we'd finished setting up the environment. However, it's unclear to me that it's safe to run `uv cache clean` during a `uv run` operation (e.g., `uv run --script` uses an environment in the cache) and, more importantly, we cannot drop non-persistent caches (e.g., from `--no-cache`) as they include the environment we're spawning the command in. Alternative to #15977 which retains release of the lock — we may want to consider that approach still but this regression needs to be resolved quickly. Closes #15989 Closes #15987 Closes #15967
|
I just removed these |
|
We should add the test case either way, the cache logic we can tune based on how much we care about |
Are there any cases we don't want to release the locks with |
|
Yeah I already gave an example there: |
4 participants
Fixes #15967