From 86a79ef19e62b8656092813dea4ca6daf85597a8 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Fri, 23 Oct 2026 22:28:02 +0100 Subject: [PATCH] fix(no-ticket): stop a stalled download from waiting without a limit The package file request in stream_download had no timeout, so a server that stopped sending data made `cloudsmith download` wait forever. The request now has a (30, 60) second connect and read timeout. A read error during the transfer gives a ClickException. An HTTP error response is now closed, so it returns its connection to the session pool. Before, the fifth download on one session waited for a free connection without a limit. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 +++ cloudsmith_cli/core/download.py | 25 ++++++++++-- cloudsmith_cli/core/tests/test_download.py | 45 ++++++++++++++++++++++ 3 files changed, 72 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0127743c7..84f7b75ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - After a SAML login, `cloudsmith auth` saves the Workspace to `config.ini`. It writes to the `[profile:NAME]` section of the selected profile, or to `[default]` when you do not select a profile. The CLI writes to the file or directory that `--config-file` gives, or else to your user-level `config.ini`. If the `config.ini` does not exist, the CLI creates it. +### Fixed + +- `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. + ## [1.30.0] - 2026-10-22 ### Added diff --git a/cloudsmith_cli/core/download.py b/cloudsmith_cli/core/download.py index 33fbc7667..cbb8679aa 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("⚠ 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 c5278c7d7..4799895ff 100644 --- a/cloudsmith_cli/core/tests/test_download.py +++ b/cloudsmith_cli/core/tests/test_download.py @@ -6,6 +6,7 @@ from unittest.mock import Mock, patch import click +import requests from cloudsmith_cli.core import download from cloudsmith_cli.core.credentials.models import CredentialResult @@ -369,6 +370,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() class TestSelectBestPackage(unittest.TestCase):