Improve _attempt_resumes_or_redownloads and update tests

Author: notatallshaw

news/13441.bugfix.rst

@@ -1 +1 @@
-Add the full file from a resume retry download to the cache.
+Resumed downloads are saved to the HTTP cache like any other normal download.

src/pip/_internal/network/download.py

@@ -270,12 +270,7 @@ def _cache_resumed_download(
         requests can use the cache.
         """
         url = download.link.url_without_fragment
-        if url.startswith("https://"):
-            adapter = self._session.adapters["https://"]
-        elif url.startswith("http://"):
-            adapter = self._session.adapters["http://"]
-        else:
-            return
+        adapter = self._session.get_adapter(url)
 
         # Check if the adapter is the CacheControlAdapter (i.e. caching is enabled)
         if not isinstance(adapter, CacheControlAdapter):
@@ -285,12 +280,9 @@ def _cache_resumed_download(
             return
 
         # Check SafeFileCache is being used
-        if not isinstance(adapter.cache, SafeFileCache):
-            logger.debug(
-                "Skipping resume download caching: "
-                "cache doesn't support separate body storage"
-            )
-            return
+        assert isinstance(
+            adapter.cache, SafeFileCache
+        ), "separate body cache not in use!"
 
         synthetic_request = PreparedRequest()
         synthetic_request.prepare(method="GET", url=url, headers={})
@@ -308,14 +300,12 @@ def _cache_resumed_download(
             preload_content=False,
         )
 
-        # Stream the file to cache
+        # Save metadata and then stream the file contents to cache.
         cache_url = adapter.controller.cache_url(url)
-        adapter.cache.set(
-            cache_url,
-            adapter.controller.serializer.dumps(
-                synthetic_request, synthetic_response, b""
-            ),
+        metadata_blob = adapter.controller.serializer.dumps(
+            synthetic_request, synthetic_response, b""
         )
+        adapter.cache.set(cache_url, metadata_blob)
         download.output_file.flush()
         with open(download.output_file.name, "rb") as f:
             adapter.cache.set_body_from_io(cache_url, f)

tests/unit/test_network_download.py

@@ -9,15 +9,14 @@
 
 from pip._internal.exceptions import IncompleteDownloadError
 from pip._internal.models.link import Link
-from pip._internal.network.cache import SafeFileCache
 from pip._internal.network.download import (
     Downloader,
     _get_http_response_size,
     _log_download,
     parse_content_disposition,
     sanitize_content_filename,
 )
-from pip._internal.network.session import CacheControlAdapter, PipSession
+from pip._internal.network.session import PipSession
 from pip._internal.network.utils import HEADERS
 
 from tests.lib.requests_mocks import MockResponse
@@ -355,8 +354,9 @@ def test_downloader(
 
 def test_resumed_download_caching(tmpdir: Path) -> None:
     """Test that resumed downloads are cached properly for future use."""
-    session = PipSession()
-    link = Link("http://example.com/foo.tgz")
+    cache_dir = tmpdir / "cache"
+    session = PipSession(cache=str(cache_dir))
+    link = Link("https://example.com/foo.tgz")
     downloader = Downloader(session, "on", resume_retries=5)
 
     # Mock an incomplete download followed by a successful resume
@@ -371,27 +371,8 @@ def test_resumed_download_caching(tmpdir: Path) -> None:
     responses = [incomplete_resp, resume_resp]
     _http_get_mock = MagicMock(side_effect=responses)
 
-    # Mock the session's adapters to have a cache controller
-    mock_adapter = MagicMock(spec=CacheControlAdapter)
-    mock_controller = MagicMock()
-    mock_adapter.controller = mock_controller
-    mock_controller.cache_url = MagicMock(return_value="cache_key")
-    mock_controller.serializer = MagicMock()
-    mock_controller.serializer.dumps = MagicMock(return_value=b"serialized_data")
-
-    # Mock the cache to be a SafeFileCache
-    mock_cache = MagicMock(spec=SafeFileCache)
-    mock_adapter.cache = mock_cache
-
-    # Create a mock for the session adapters
-    adapters_mock = MagicMock()
-    adapters_mock.__getitem__ = MagicMock(return_value=mock_adapter)
-
-    with (
-        patch.object(Downloader, "_http_get", _http_get_mock),
-        patch.object(session, "adapters", adapters_mock),
-    ):
-
+    with patch.object(Downloader, "_http_get", _http_get_mock):
+        # Perform the download (incomplete then resumed)
         filepath, _ = downloader(link, str(tmpdir))
 
         # Verify the file was downloaded correctly
@@ -400,18 +381,9 @@ def test_resumed_download_caching(tmpdir: Path) -> None:
             expected_bytes = b"0cfa7e9d-1868-4dd7-9fb3-f2561d5dfd89"
             assert downloaded_bytes == expected_bytes
 
-        # Verify that cache.set was called for metadata
-        mock_cache.set.assert_called_once()
-
-        # Verify that set_body_from_io was called for streaming the body
-        mock_cache.set_body_from_io.assert_called_once()
-
-        # Verify the call arguments
-        set_call_args = mock_cache.set.call_args
-        assert set_call_args[0][0] == "cache_key"  # First argument should be cache_key
-
-        set_body_call_args = mock_cache.set_body_from_io.call_args
-
-        assert set_body_call_args[0][0] == "cache_key"
-        assert hasattr(set_body_call_args[0][1], "read")
-        assert set_body_call_args[0][1].name == filepath
+        # Verify that the cache directory was created and contains cache files
+        # The resumed download should have been cached for future use
+        assert cache_dir.exists()
+        cache_files = list(cache_dir.rglob("*"))
+        # Should have cache files (both metadata and body files)
+        assert len([f for f in cache_files if f.is_file()]) == 2