diff --git a/CHANGELOG.md b/CHANGELOG.md index ffbf17a4..e43b2f90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -90,6 +90,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - `cloudsmith credential-helper install` and `uninstall` no longer rewrite a config file when they make no change to its data. Previously, the CLI always rewrote a file in a different formatting, such as the tab-indented `config.json` that Docker writes. The command also made a `.bak` copy and reported changes that it did not make. - When the API host name or the proxy host name does not exist, for example because of a typo in `--api-host`, commands now fail immediately. Previously, they retried for approximately 62 seconds without output. The CLI still retries a DNS failure that the operating system reports as temporary. - When the CLI waits before it retries a failed connection or a 5xx response, it now writes `Request failed: retrying in N second(s) ...` to stderr. Previously, it waited without output. +- `cloudsmith download` now stops with an error when the server stops sending the file. Before, the command waited without a limit. Each connection attempt has a 30-second limit. Each read from the server has a 60-second limit, so large files still download. +- `cloudsmith download` no longer stops responding after four files fail with an HTTP error. Before, the next file waited for a free connection without a limit. - `cloudsmith mcp start` now stops without an error when the MCP client closes the connection while the server writes a reply. Previously, the command stopped with `BrokenPipeError` and exit status 1. - The CLI now exits with status 1 when you interrupt it with Ctrl+C after the reader of its stderr has closed the pipe. Before, the CLI sent an error report and exited with status 120. - `cloudsmith check rates` no longer stops with `TypeError` when the API does not give a value for an interval, limit, remaining count, reset time or throttled status. The command now shows `None` for that value. diff --git a/cloudsmith_cli/core/download.py b/cloudsmith_cli/core/download.py index c2db471e..edebb190 100644 --- a/cloudsmith_cli/core/download.py +++ b/cloudsmith_cli/core/download.py @@ -4,6 +4,7 @@ import hashlib import os import sys +from collections.abc import Iterator import click import requests @@ -15,6 +16,9 @@ from .api.packages import get_packages_api, list_packages from .session import create_requests_session +# The read timeout limits the wait for each socket read, not the full transfer. +DOWNLOAD_TIMEOUT_SECONDS: tuple[float, float] = (30.0, 60.0) + def resolve_auth( opts, api_key_opt: str | None = None @@ -466,9 +470,16 @@ def stream_download( # Attempt download with configured auth try: - response = session.get(url, headers=request_headers, auth=auth, stream=True) + response = session.get( + url, + headers=request_headers, + auth=auth, + stream=True, + timeout=DOWNLOAD_TIMEOUT_SECONDS, + ) response.raise_for_status() except requests.exceptions.HTTPError as e: + e.response.close() raise click.ClickException( f"Failed to download package: HTTP {e.response.status_code}" ) @@ -490,7 +501,7 @@ def stream_download( with click.progressbar( length=total_size, label="Downloading" ) as progress_bar: - for chunk in response.iter_content(chunk_size=chunk_size): + for chunk in _iter_chunks(response, chunk_size): if chunk: f.write(chunk) downloaded += len(chunk) @@ -499,7 +510,7 @@ def stream_download( # No progress bar for unknown size or quiet mode if not quiet: click.echo(f"Downloading to {outfile}...") - for chunk in response.iter_content(chunk_size=chunk_size): + for chunk in _iter_chunks(response, chunk_size): if chunk: f.write(chunk) downloaded += len(chunk) @@ -516,6 +527,14 @@ def stream_download( click.secho("Warning: Checksum mismatch", fg="yellow", err=True) +def _iter_chunks(response: requests.Response, chunk_size: int) -> Iterator[bytes]: + """Yield the response body, and raise a ClickException if a read fails.""" + try: + yield from response.iter_content(chunk_size=chunk_size) + except requests.exceptions.RequestException as exc: + raise click.ClickException(f"Failed to download package: {exc!s}") from exc + + def _select_best_package(packages: list[dict]) -> dict: """Select the best package from multiple matches.""" diff --git a/cloudsmith_cli/core/tests/test_download.py b/cloudsmith_cli/core/tests/test_download.py index bb755eb4..9bf3177d 100644 --- a/cloudsmith_cli/core/tests/test_download.py +++ b/cloudsmith_cli/core/tests/test_download.py @@ -9,6 +9,7 @@ import click import pytest +import requests from click.testing import CliRunner from cloudsmith_cli.core import download @@ -373,6 +374,50 @@ def test_stream_download_success( # Verify file was written mock_file.write.assert_any_call(b"data1") mock_file.write.assert_any_call(b"data2") + self.assertEqual( + self.session.get.call_args.kwargs["timeout"], + download.DOWNLOAD_TIMEOUT_SECONDS, + ) + + @patch("os.makedirs") + @patch("click.open_file") + def test_stream_download_stalls_mid_stream(self, mock_open_file, mock_makedirs): + """Test a read timeout between chunks gives a ClickException.""" + + def stalled_body(chunk_size): + yield b"data1" + raise requests.exceptions.ConnectionError("Read timed out.") + + mock_response = Mock() + mock_response.headers = {} + mock_response.iter_content.side_effect = stalled_body + self.session.get.return_value = mock_response + + with self.assertRaisesRegex(click.ClickException, "Read timed out"): + download.stream_download( + "https://example.com/file.deb", + "/path/to/file.deb", + self.session, + overwrite=True, + quiet=True, + ) + + def test_stream_download_http_error_closes_response(self): + """Test an HTTP error closes the response and gives a ClickException.""" + mock_response = Mock(status_code=404) + mock_response.raise_for_status.side_effect = requests.exceptions.HTTPError( + response=mock_response + ) + self.session.get.return_value = mock_response + + with self.assertRaisesRegex(click.ClickException, "HTTP 404"): + download.stream_download( + "https://example.com/file.deb", + "/path/to/file.deb", + self.session, + overwrite=True, + ) + mock_response.close.assert_called_once_with() @pytest.mark.windows @patch("cloudsmith_cli.core.download._verify_checksum", return_value=True)