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
Prevent Cache.download()
deleting an expired file before knowing that it can download a new version of the file
#7265
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR @samish55
I think you need to refactor the logic a little bit to be sure the old file doesn't get deleted.
4f5b086
to
d69fa1b
Compare
Thanks for the code. I think ideally we need a unit test to make sure this behavior is working. It is going to be tricky to do as you will need to monkey patch several calls (I think) to fake the download and make it fail. |
Cache.download()
deleting an expired file before knowing that it can download a new version of the file
0e03f22
to
c188102
Compare
@@ -40,56 +39,60 @@ def download(self, urls, namespace='', redownload=False): | |||
""" | |||
Downloads the files from the urls. | |||
|
|||
The overall flow of this function is: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now just in the code as a comment instead of in both places.
if cache_details and (redownload or self._has_expired(cache_details)): | ||
self._storage.delete_by_key('url', cache_details['url']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is logic problem here:
- We do not remove any old files from disk
- If the file changes on disk, we could remove it here but then it will break logic in the manager as we need to hash the file a second time.
PR Description
I've attempted to handle issue #7249. The expired file won't be deleted until the file is successfully downloaded and if the download fails(for some reason) an error warning is emitted about working with an expired file in cache.
Fixes #7249