Download Failure Resilience

changes/753.feature.rst

@@ -0,0 +1 @@
+Briefcase is more resilient to file download failures by discarding partially downloaded files.

src/briefcase/integrations/download.py

@@ -1,3 +1,6 @@
+import os
+import tempfile
+from contextlib import suppress
 from email.message import Message
 from urllib.parse import urlparse
 
@@ -74,21 +77,8 @@ def file(self, url, download_path, role=None):
             if filename.exists():
                 self.tools.logger.info(f"{cache_name} already downloaded")
             else:
-                # We have meaningful content, and it hasn't been cached previously,
-                # so save it in the requested location
                 self.tools.logger.info(f"Downloading {cache_name}...")
-                with filename.open("wb") as f:
-                    total = response.headers.get("content-length")
-                    if total is None:
-                        f.write(response.content)
-                    else:
-                        progress_bar = self.tools.input.progress_bar()
-                        task_id = progress_bar.add_task("Downloader", total=int(total))
-                        with progress_bar:
-                            for data in response.iter_content(chunk_size=1024 * 1024):
-                                f.write(data)
-                                progress_bar.update(task_id, advance=len(data))
-
+                self._fetch_and_write_content(response, filename)
         except requests_exceptions.ConnectionError as e:
             if role:
                 description = role
@@ -97,3 +87,38 @@ def file(self, url, download_path, role=None):
             raise NetworkFailure(f"download {description}") from e
 
         return filename
+
+    def _fetch_and_write_content(self, response, filename):
+        """Write the content from the Requests response to file.
+
+        The data is written in to a temporary file in a filesystem location
+        designated by the platform. This avoids partially downloaded files
+        masquerading as complete downloads in later Briefcase runs.
+        The temporary file is only moved to ``filename`` if the download
+        is successful; otherwise, it is deleted.
+
+        :param response: ``requests.Response``
+        :param filename: full filesystem path to save data
+        """
+        try:
+            with tempfile.NamedTemporaryFile(delete=False) as temp_file:
+                total = response.headers.get("content-length")
+                if total is None:
+                    temp_file.write(response.content)
+                else:
+                    progress_bar = self.tools.input.progress_bar()
+                    task_id = progress_bar.add_task("Downloader", total=int(total))
+                    with progress_bar:
+                        for data in response.iter_content(chunk_size=1024 * 1024):
+                            temp_file.write(data)
+                            progress_bar.update(task_id, advance=len(data))
+
+            self.tools.shutil.move(temp_file.name, filename)
+            # retrieving the current umask requires changing it...
+            os.umask(current_umask := os.umask(0o022))
+            self.tools.os.chmod(filename, 0o664 & ~current_umask)
+        finally:
+            # Ensure the temporary file is deleted; this file may still
+            # exist if the download fails or the user sends CTRL+C.
+            with suppress(OSError):
+                self.tools.os.remove(temp_file.name)

tests/integrations/download/test_Download__file.py

@@ -1,3 +1,4 @@
+import shutil
 from unittest import mock
 
 import pytest
@@ -16,6 +17,8 @@
 @pytest.fixture
 def mock_tools(mock_tools) -> ToolCache:
     mock_tools.requests = mock.MagicMock(spec_set=requests)
+    # Restore move so the temporary file can be moved after downloaded
+    mock_tools.shutil.move = mock.MagicMock(wraps=shutil.move)
     return mock_tools
 
 
@@ -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)
+    mock_tools.os.chmod.assert_called_with(filename, mock.ANY)
+    mock_tools.os.remove.assert_called_with(mock.ANY)
+
     # File content is as expected
     with (mock_tools.base_path / "downloads" / "something.zip").open() as f:
         assert f.read() == "all content"
@@ -122,6 +130,11 @@ def test_new_download_chunked(mock_tools):
     # The filename is derived from the URL
     assert filename == mock_tools.base_path / "something.zip"
 
+    # Temporary file was upgraded to intended destination
+    mock_tools.shutil.move.assert_called_with(mock.ANY, filename)
+    mock_tools.os.chmod.assert_called_with(filename, mock.ANY)
+    mock_tools.os.remove.assert_called_with(mock.ANY)
+
     # The downloaded file exists, and content is as expected
     assert filename.exists()
     with (mock_tools.base_path / "something.zip").open() as f:
@@ -161,6 +174,11 @@ def test_already_downloaded(mock_tools):
     assert filename == existing_file
     assert filename.exists()
 
+    # Temporary file was not moved
+    mock_tools.shutil.move.assert_not_called()
+    mock_tools.os.chmod.assert_not_called()
+    mock_tools.os.remove.assert_not_called()
+
 
 def test_missing_resource(mock_tools):
     response = mock.MagicMock()
@@ -185,6 +203,11 @@ def test_missing_resource(mock_tools):
     # The file doesn't exist as a result of the download failure
     assert not (mock_tools.base_path / "something.zip").exists()
 
+    # Temporary file was not moved
+    mock_tools.shutil.move.assert_not_called()
+    mock_tools.os.chmod.assert_not_called()
+    mock_tools.os.remove.assert_not_called()
+
 
 def test_bad_resource(mock_tools):
     response = mock.MagicMock()
@@ -209,6 +232,11 @@ def test_bad_resource(mock_tools):
     # The file doesn't exist as a result of the download failure
     assert not (mock_tools.base_path / "something.zip").exists()
 
+    # Temporary file was not moved
+    mock_tools.shutil.move.assert_not_called()
+    mock_tools.os.chmod.assert_not_called()
+    mock_tools.os.remove.assert_not_called()
+
 
 def test_get_connection_error(mock_tools):
     """NetworkFailure raises if requests.get() errors."""
@@ -233,6 +261,11 @@ def test_get_connection_error(mock_tools):
     # The file doesn't exist as a result of the download failure
     assert not (mock_tools.base_path / "something.zip").exists()
 
+    # Temporary file was not moved
+    mock_tools.shutil.move.assert_not_called()
+    mock_tools.os.chmod.assert_not_called()
+    mock_tools.os.remove.assert_not_called()
+
 
 def test_iter_content_connection_error(mock_tools):
     """NetworkFailure raised if response.iter_content() errors."""
@@ -260,6 +293,11 @@ def test_iter_content_connection_error(mock_tools):
     # The file doesn't exist as a result of the download failure
     assert not (mock_tools.base_path / "something.zip").exists()
 
+    # Temporary file was not moved but it was deleted
+    mock_tools.shutil.move.assert_not_called()
+    mock_tools.os.chmod.assert_not_called()
+    mock_tools.os.remove.assert_called_with(mock.ANY)
+
 
 def test_content_connection_error(mock_tools):
     """NetworkFailure raised if response.content errors."""
@@ -289,3 +327,8 @@ def test_content_connection_error(mock_tools):
 
     # The file doesn't exist as a result of the download failure
     assert not (mock_tools.base_path / "something.zip").exists()
+
+    # Temporary file was not moved but it was deleted
+    mock_tools.shutil.move.assert_not_called()
+    mock_tools.os.chmod.assert_not_called()
+    mock_tools.os.remove.assert_called_with(mock.ANY)