Merge 02e36211ff into 7b4d4ed672

changelogs/fragments/ansible-galaxy-retry-IncompleteRead-download.yml

@@ -0,0 +1,6 @@
+bugfixes:
+- >-
+  ansible-galaxy - Fix attempting to download the collection again if the
+  response from the server is shorter than expected, instead of failing due to
+  the mismatched artifact hash on the first attempt.
+  (https://github.com/ansible/ansible/pull/86025)

lib/ansible/galaxy/collection/concrete_artifact_manager.py

@@ -14,6 +14,7 @@ import yaml
 
 from contextlib import contextmanager
 from hashlib import sha256
+from http.client import IncompleteRead, HTTPResponse
 from urllib.error import URLError
 from urllib.parse import urldefrag
 from shutil import rmtree
@@ -494,7 +495,10 @@ def _download_file(url, b_path, expected_hash, validate_certs, token=None, timeo
     )
 
     with open(b_file_path, 'wb') as download_file:  # type: t.BinaryIO
+        try:
             actual_hash = _consume_file(resp, write_to=download_file)
+        except IncompleteRead as orig_exc:
+            raise AnsibleError(f"Downloading {url} failed") from orig_exc
 
     if expected_hash:
         display.vvvv(
@@ -508,17 +512,29 @@ def _download_file(url, b_path, expected_hash, validate_certs, token=None, timeo
     return b_file_path
 
 
-def _consume_file(read_from, write_to=None):
-    # type: (t.BinaryIO, t.BinaryIO) -> str
+def _consume_file(read_from: t.BinaryIO | HTTPResponse, write_to: t.BinaryIO | None = None) -> str:
     bufsize = 65536
     sha256_digest = sha256()
-    data = read_from.read(bufsize)
-    while data:
+    length = getattr(read_from, 'length', None)
+    actual_length = 0
+    remaining = None
+    while True:
+        if isinstance(length, int) and (remaining := length - actual_length) <= 0:
+            break
+
+        if not (data := read_from.read(min(bufsize, remaining or bufsize))):
+            break
+
         if write_to is not None:
             write_to.write(data)
             write_to.flush()
+
         sha256_digest.update(data)
-        data = read_from.read(bufsize)
+        actual_length += len(data)
+
+    if isinstance(length, int) and length > actual_length:
+        read_len = b' ' * actual_length if actual_length else b''
+        raise IncompleteRead(read_len, length - actual_length)
 
     return sha256_digest.hexdigest()
 

test/units/cli/test_galaxy.py

@@ -918,10 +918,21 @@ def test_collection_install_in_collection_dir(collection_install, monkeypatch):
     assert mock_install.call_args[0][6] is False  # force_deps
 
 
-def test_collection_install_with_url(monkeypatch, collection_install):
+def test_collection_install_with_url(
+    monkeypatch: pytest.MonkeyPatch,
+    collection_install: tuple[MagicMock, MagicMock, str]
+) -> None:
     mock_install, dummy, output_dir = collection_install
 
-    mock_open = MagicMock(return_value=BytesIO())
+    class MockHTTPResponse:
+        def __init__(self) -> None:
+            self._stream = BytesIO()
+            self.length = 'UNKNOWN'
+
+        def read(self, size: int = -1) -> bytes:
+            return self._stream.read(size)
+
+    mock_open = MagicMock(return_value=MockHTTPResponse())
     monkeypatch.setattr(collection.concrete_artifact_manager, 'open_url', mock_open)
 
     mock_metadata = MagicMock(return_value={'namespace': 'foo', 'name': 'bar', 'version': 'v1.0.0'})

test/units/galaxy/test_collection.py

@@ -890,7 +890,26 @@ def test_publish_with_wait(galaxy_server, collection_artifact, monkeypatch):
         % galaxy_server.api_server
 
 
-def test_download_file(tmp_path_factory, monkeypatch):
+class MockHTTPResponses:
+    def __init__(self, responses: list[tuple[bytes, int | None]]) -> None:
+        self._body, self.length = responses[0]
+        self._responses = responses[1:]
+        self._stream = BytesIO(self._body)
+
+    def read(self, size: int = -1) -> bytes:
+        data = self._stream.read(size)
+
+        if not data:
+            # reset the stream to simulate retries
+            if self._responses:
+                self._body, self.length = self._responses[0]
+                del self._responses[0]
+            self._stream = BytesIO(self._body)
+
+        return data
+
+
+def test_download_file(tmp_path_factory: pytest.TempPathFactory, monkeypatch: pytest.MonkeyPatch) -> None:
     temp_dir = to_bytes(tmp_path_factory.mktemp('test-ÅÑŚÌβŁÈ Collections'))
 
     data = b"\x00\x01\x02\x03"
@@ -898,7 +917,7 @@ def test_download_file(tmp_path_factory, monkeypatch):
     sha256_hash.update(data)
 
     mock_open = MagicMock()
-    mock_open.return_value = BytesIO(data)
+    mock_open.return_value = MockHTTPResponses([(data, len(data))])
     monkeypatch.setattr(collection.concrete_artifact_manager, 'open_url', mock_open)
 
     expected = temp_dir
@@ -913,13 +932,13 @@ def test_download_file(tmp_path_factory, monkeypatch):
     assert mock_open.mock_calls[0][1][0] == 'http://google.com/file'
 
 
-def test_download_file_hash_mismatch(tmp_path_factory, monkeypatch):
+def test_download_file_hash_mismatch(tmp_path_factory: pytest.TempPathFactory, monkeypatch: pytest.MonkeyPatch) -> None:
     temp_dir = to_bytes(tmp_path_factory.mktemp('test-ÅÑŚÌβŁÈ Collections'))
 
     data = b"\x00\x01\x02\x03"
 
     mock_open = MagicMock()
-    mock_open.return_value = BytesIO(data)
+    mock_open.return_value = MockHTTPResponses([(data, len(data))])
     monkeypatch.setattr(collection.concrete_artifact_manager, 'open_url', mock_open)
 
     expected = "Mismatch artifact hash with downloaded file"
@@ -927,6 +946,35 @@ def test_download_file_hash_mismatch(tmp_path_factory, monkeypatch):
         collection._download_file('http://google.com/file', temp_dir, 'bad', True)
 
 
+def test_download_file_incomplete_read(
+    tmp_path_factory: pytest.TempPathFactory,
+    monkeypatch: pytest.MonkeyPatch
+) -> None:
+    temp_dir = to_bytes(tmp_path_factory.mktemp('test-ÅÑŚÌβŁÈ Collections'))
+
+    incomplete_data = b"\x00\x01\x02\x03"
+    data = incomplete_data + b"\x04"
+    sha256_hash = sha256(data)
+
+    # Test retries succeed
+    responses = [(incomplete_data, len(data)), (data, len(data))]
+    mock_open = MagicMock(return_value=MockHTTPResponses(responses))
+    monkeypatch.setattr(collection.concrete_artifact_manager, 'open_url', mock_open)
+
+    collection._download_file('http://google.com/file', temp_dir, sha256_hash.hexdigest(), True)
+
+    # Test error is correct
+    mock_open.return_value = MockHTTPResponses([(data, len(data) + 1)])
+    monkeypatch.setattr(collection.concrete_artifact_manager, 'open_url', mock_open)
+
+    expected_error = re.escape(
+        "Downloading http://google.com/file failed: "
+        "IncompleteRead(5 bytes read, 1 more expected)"
+    )
+    with pytest.raises(AnsibleError, match=expected_error):
+        collection._download_file('http://google.com/file', temp_dir, sha256_hash.hexdigest(), True)
+
+
 def test_extract_tar_file_invalid_hash(tmp_tarfile):
     temp_dir, tfile, filename, dummy = tmp_tarfile