fix: deadlock for duplicate packages from different sources#5447
fix: deadlock for duplicate packages from different sources#5447Hofer-Julian merged 7 commits intoprefix-dev:mainfrom
Conversation
| ) -> String { | ||
| match manifest_source { | ||
| CanonicalSourceLocation::Url(url) => { | ||
| let mut hasher = Xxh3::new(); |
There was a problem hiding this comment.
can we make this a method on CanonicalSourceLocation?
I think it will be a nice API
CanonicalSourceLocation::unique_key(something, source)
There was a problem hiding this comment.
Good suggestions, moved to CanonicalSourceCodeLocation::cache_unique_key
| // Check the source metadata cache, short circuit if there is a cache hit that | ||
| // is still fresh. | ||
| let cache_key = self.cache_shard(); | ||
| let cache_key = BuildBackendMetadataCacheShard { |
There was a problem hiding this comment.
should we adjust self.cache_shard() or make separate function for it?
self.manifest_cache_shard?
There was a problem hiding this comment.
It requires input from the current function and we dont need it anywhere else. So Im inclined to leave as is.
| /// resolved location is not. | ||
| pub build_source: PinnedSourceSpec, | ||
| /// | ||
| /// If this field is none, its the same as the `manifest_source`. |
There was a problem hiding this comment.
Ok now I'm getting really curios here!
Could we somehow express it with a separate type? So I dont need to manifest_source != build_source in order to know if I have a build_source or not.
There was a problem hiding this comment.
I added a type and used it everywhere!
| // Get the skip_cache flag from the build backend metadata | ||
| let skip_cache = build_backend_metadata.skip_cache; | ||
|
|
||
| let shard = SourceMetadataCacheShard { |
There was a problem hiding this comment.
can we have a method for it?
There was a problem hiding this comment.
Id like to keep it here because it uses local variables.
| } | ||
|
|
||
| let records: Vec<SourceRecord> = futures.try_collect().await?; | ||
| let records: Vec<_> = futures.try_collect().await?; |
There was a problem hiding this comment.
do we need typing Vec here?
There was a problem hiding this comment.
Yes otherwise in the line below we do not know how to resolve records.is_empty
|
@nichmor Ready for another review! |
f5cea9f to
bab2f99
Compare
bab2f99 to
98072ba
Compare
Description
Fixes an issue where two source builds of the same package but from different locations would deadlock. This PR ensures that caches are appropriately seperated.
This also removes the git reference from caches (using
CanonicalSourceLocation), because only the commit really matters.How Has This Been Tested?
Tested using a reproducer from @lucascolley
AI Disclosure
Tools: Claude Code Opus 4.5
Checklist:
schema/model.py.