Download Failure Resilience #956
Conversation
- Instead of downloading directly to the Briefcase cache, files are downloaded to the platform's designated temporary directory. - If the download succeeds, the temporary file is moved in to the Briefcase cache. - In most cases, the temporary file will be deleted is the download fails; one such exception may be Briefcase receiving SIGKILL.
rmartin16
changed the title
rmartin16
force-pushed
the
cleanup-failed-downloads
branch
from
5c2bcb8 to
569b50d
Compare
| # retrieving the current umask requires changing it... | ||
| os.umask(current_umask := os.umask(0o022)) | ||
| self.tools.os.chmod(filename, 0o664 & ~current_umask) |
There was a problem hiding this comment.
The NamedTemporaryFile file is only created with 600 permissions. So, all this umask stuff restores the previous behavior from filename.open("wb").
There was a problem hiding this comment.
A longer explanatory comment would be helpful here. The interplay between the 0o022 mask and the and not with 0o644 wan't clear to me.
There's also no test that the resulting file mask is actually followed; there's code coverage, but no real check of the logic.
There was a problem hiding this comment.
Added (some probably overly verbose) comments about umask. I also tried to implement file permission verification in the tests.....things are really wonky on windows, though...
- By default, temporary files are created in a platform-specific temporary directory. This directory may be on a different filesystem than the Briefcase cache and make the attempt to move the file to its final location a copy/rm operation instead. - Additionally, some of the downloads are large and if the user has put the Briefcase cache on a different filesystem, we should only create files in that location.
|
As mentioned in #753:
Right now, this PR doesn't address the possibility of So, should I also make updates to glob the Briefcase cache directory for Alternatively, perhaps this type of cleanup would be more appropriate as part of "[a] user-accessible mechanism to manually flush the download cache". |
| @@ -86,6 +89,11 @@ def test_new_download_oneshot(mock_tools, url, content_disposition): | |||
| # The filename is derived from the URL or header | |||
| assert filename == mock_tools.base_path / "downloads" / "something.zip" | |||
|
|
|||
| # Temporary file was upgraded to intended destination | |||
| mock_tools.shutil.move.assert_called_with(mock.ANY, filename) | |||
There was a problem hiding this comment.
While this test passes, it's not especially resilient - mock.ANY really does allow any content. This initial "mock.ANY" call is definitely required to verify that a filename was used, but it would be desirable to add checks that the same temporary filename was used for all the subsequent calls.
There was a problem hiding this comment.
That's fair; I was initially struggling with how to introspect information about the temporary file and I went too far towards "assume it's working if everything else is working." I think my changes now verify the logic is working as designed.
| # retrieving the current umask requires changing it... | ||
| os.umask(current_umask := os.umask(0o022)) | ||
| self.tools.os.chmod(filename, 0o664 & ~current_umask) |
There was a problem hiding this comment.
A longer explanatory comment would be helpful here. The interplay between the 0o022 mask and the and not with 0o644 wan't clear to me.
There's also no test that the resulting file mask is actually followed; there's code coverage, but no real check of the logic.
- Along with explaining umask usage, the default file permissions were updated from 664 to 666 to better mirror `open()`.
Changes
Issues
The other recommendations in #753 are a bit more complicated....so, I didn't implement any of them here so we can at least get this win.
PR Checklist: