From 9cea2ef0bd9e4d2a4a147dfd1bae5c6f84c19457 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Fri, 23 Oct 2026 22:39:34 +0100 Subject: [PATCH 1/5] fix(no-ticket): set a default timeout on each API request The v1 SDK calls sent no timeout, so a host that accepts the connection and does not reply blocked the command forever. RestClient.request now uses a 30-second timeout when the caller gives none. A POST gets only the connection timeout, because a retry after a read timeout can create a second entry. The v2 client now uses this default instead of its own copy of the rule. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 ++ cloudsmith_cli/core/api/v2.py | 11 ----- cloudsmith_cli/core/rest.py | 51 +++++++++++++++++----- cloudsmith_cli/core/tests/test_rest.py | 58 +++++++++++++++++++++++++- cloudsmith_cli/core/tests/test_v2.py | 34 --------------- 5 files changed, 102 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0127743c..8363b430 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ 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 + +- An API request no longer waits without a limit when the API host accepts the connection but does not reply. A request now fails after 30 seconds without data, and the CLI retries it 5 times by default (`--error-retry-max`). A POST request has only the 30-second connection timeout, because a retry after a read timeout can create a second entry. File uploads and downloads do not change. + ## [1.30.0] - 2026-10-22 ### Added diff --git a/cloudsmith_cli/core/api/v2.py b/cloudsmith_cli/core/api/v2.py index 225cc794..4f92804c 100644 --- a/cloudsmith_cli/core/api/v2.py +++ b/cloudsmith_cli/core/api/v2.py @@ -11,8 +11,6 @@ from ..rest import RestClient from .exceptions import ApiException, catch_raise_api_exception -REQUEST_TIMEOUT_SECONDS = 30 - class V2Client: """Small client for v2 endpoints not yet present in cloudsmith_api.""" @@ -56,14 +54,6 @@ def _is_same_origin(url, host): return (target.scheme, target.netloc) == (configured.scheme, configured.netloc) -def _timeout(method): - # A retry after a read timeout sends the request again, so a POST gets - # only a connect timeout and cannot create a second entry. - if method.upper() == "POST": - return (REQUEST_TIMEOUT_SECONDS, None) - return REQUEST_TIMEOUT_SECONDS - - def request(client, method, *path_parts, query_params=None, body=None): """Send a request to the v2 endpoint that `path_parts` names.""" url = _build_url(client.config, *path_parts) @@ -82,7 +72,6 @@ def request_url(client, method, url, query_params=None, body=None): query_params=query_params, headers=_build_headers(client), body=body, - _request_timeout=_timeout(method), ) ratelimits.maybe_rate_limit(client, response.getheaders()) diff --git a/cloudsmith_cli/core/rest.py b/cloudsmith_cli/core/rest.py index 4b6f6219..99992922 100644 --- a/cloudsmith_cli/core/rest.py +++ b/cloudsmith_cli/core/rest.py @@ -4,6 +4,8 @@ import json import logging import re +from collections.abc import Mapping, Sequence +from typing import Any from urllib.parse import urlencode import requests @@ -14,6 +16,33 @@ logger = logging.getLogger(__name__) +REQUEST_TIMEOUT_SECONDS = 30 + +Timeout = float | tuple[float | None, float | None] + + +def _is_seconds(value: object) -> bool: + return isinstance(value, int | float) and not isinstance(value, bool) and value > 0 + + +def _resolve_timeout(method: str, timeout: Timeout | None) -> Timeout: + """Return the timeout for a request, or the default when there is none.""" + if timeout is None: + # A retry after a read timeout sends the request again, so a POST gets + # only a connect timeout and cannot create a second entry. + if method == "POST": + return (REQUEST_TIMEOUT_SECONDS, None) + return REQUEST_TIMEOUT_SECONDS + if _is_seconds(timeout): + return timeout + if ( + isinstance(timeout, tuple) + and len(timeout) == 2 + and all(value is None or _is_seconds(value) for value in timeout) + ): + return timeout + raise ApiException(status=0, reason=f"Invalid request timeout: {timeout!r}") + class RestResponse(io.IOBase): """A urllib3 adapter for a requests response.""" @@ -56,15 +85,15 @@ def __init__(self, *args, **kwargs): def request( self, - method, - url, - query_params=None, - headers=None, - body=None, - post_params=None, - _preload_content=True, - _request_timeout=None, - ): + method: str, + url: str, + query_params: Mapping[str, Any] | Sequence[tuple[str, Any]] | None = None, + headers: dict[str, str] | None = None, + body: Any = None, + post_params: Mapping[str, Any] | Sequence[tuple[str, Any]] | None = None, + _preload_content: bool = True, + _request_timeout: Timeout | None = None, + ) -> RestResponse: """ :param method: http request method :param url: http request url @@ -78,10 +107,12 @@ def request( reading/decoding response data. Default is True. :param _request_timeout: timeout setting for this request. If one number provided, it will be total request timeout. It can also be a pair (tuple) of (connection, read) timeouts. + Without it, the request uses REQUEST_TIMEOUT_SECONDS, but a POST gets no read timeout. """ # Based on the RESTClientObject class generated by Swagger method = method.upper() assert method in ["GET", "HEAD", "DELETE", "POST", "PUT", "PATCH", "OPTIONS"] + timeout = _resolve_timeout(method, _request_timeout) post_params = post_params or {} headers = headers or {} @@ -117,7 +148,7 @@ def request( resp = self.session.request( method, url, - timeout=_request_timeout, + timeout=timeout, stream=not _preload_content, headers=headers, **request_kwargs, diff --git a/cloudsmith_cli/core/tests/test_rest.py b/cloudsmith_cli/core/tests/test_rest.py index 519a5d79..b17646d4 100644 --- a/cloudsmith_cli/core/tests/test_rest.py +++ b/cloudsmith_cli/core/tests/test_rest.py @@ -3,10 +3,11 @@ import httpretty import pytest import requests +from cloudsmith_api.rest import ApiException from urllib3.exceptions import MaxRetryError, SSLError from ..api.init import initialise_api -from ..rest import RestClient +from ..rest import REQUEST_TIMEOUT_SECONDS, RestClient from ..session import create_requests_session @@ -57,6 +58,61 @@ def test_implicit_retry_for_status_codes(self): assert len(httpretty.latest_requests()) == 6 assert r.status == 200 + @staticmethod + def _sent_timeouts(monkeypatch, client): + timeouts = [] + original_request = client.session.request + + def _record_timeout(*args, **kwargs): + timeouts.append(kwargs.get("timeout")) + return original_request(*args, **kwargs) + + monkeypatch.setattr(client.session, "request", _record_timeout) + return timeouts + + @pytest.mark.parametrize( + "method, expected_timeout", + [ + ("GET", REQUEST_TIMEOUT_SECONDS), + ("POST", (REQUEST_TIMEOUT_SECONDS, None)), + ], + ) + @httpretty.activate(allow_net_connect=False) + def test_sets_a_default_timeout(self, monkeypatch, method, expected_timeout): + """A stalled API does not block the CLI forever. + + A POST gets no read timeout, because the retry after a read timeout + sends the POST again and can create a second entry. + """ + initialise_api() + client = RestClient() + httpretty.register_uri(method, "https://test.site", body="{}") + timeouts = self._sent_timeouts(monkeypatch, client) + + client.request(method, "https://test.site") + + assert timeouts == [expected_timeout] + + @httpretty.activate(allow_net_connect=False) + def test_keeps_an_explicit_timeout(self, monkeypatch): + initialise_api() + client = RestClient() + httpretty.register_uri("GET", "https://test.site", body="{}") + timeouts = self._sent_timeouts(monkeypatch, client) + + client.request("GET", "https://test.site", _request_timeout=(5, 120)) + + assert timeouts == [(5, 120)] + + def test_rejects_an_invalid_timeout(self): + initialise_api() + client = RestClient() + + with pytest.raises(ApiException) as raised: + client.request("GET", "https://test.site", _request_timeout=(5, 10, 15)) + + assert raised.value.status == 0 + class TestCreateRequestsSession: @pytest.fixture(autouse=True) diff --git a/cloudsmith_cli/core/tests/test_v2.py b/cloudsmith_cli/core/tests/test_v2.py index 93e7d9df..1f7209b2 100644 --- a/cloudsmith_cli/core/tests/test_v2.py +++ b/cloudsmith_cli/core/tests/test_v2.py @@ -13,7 +13,6 @@ from ..api.exceptions import ApiException from ..api.init import initialise_api from ..credentials.models import CredentialResult -from ..rest import RestClient API_HOST = "https://api.example.com" RESOURCE_URL = f"{API_HOST}/v2/things/" @@ -108,39 +107,6 @@ def test_request_does_not_send_credentials_through_a_redirect_to_another_host( assert "Authorization" not in headers -@pytest.mark.parametrize( - "method, expected_timeout", - [ - ("GET", v2.REQUEST_TIMEOUT_SECONDS), - ("PATCH", v2.REQUEST_TIMEOUT_SECONDS), - ("DELETE", v2.REQUEST_TIMEOUT_SECONDS), - ("POST", (v2.REQUEST_TIMEOUT_SECONDS, None)), - ], -) -@httpretty.activate(allow_net_connect=False) -def test_request_sets_a_timeout(monkeypatch, method, expected_timeout): - """A stalled API does not block the CLI forever. - - A POST gets no read timeout, because the retry after a read timeout - sends the POST again and can create a second entry. - """ - httpretty.register_uri( - method, RESOURCE_URL, body=json.dumps({"results": []}), status=200 - ) - timeouts = [] - original_request = RestClient.request - - def _record_timeout(self, *args, **kwargs): - timeouts.append(kwargs.get("_request_timeout")) - return original_request(self, *args, **kwargs) - - monkeypatch.setattr(RestClient, "request", _record_timeout) - - v2.request(v2.get_client(API_KEY), method, "things") - - assert timeouts == [expected_timeout] - - @httpretty.activate(allow_net_connect=False) def test_request_sends_the_configured_user_agent_and_headers(): """The --api-user-agent and --api-headers values reach the v2 API.""" From 699fdf3d39432f58a55a87c5a37f07bca62ce18e Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Sat, 24 Oct 2026 18:24:23 +0100 Subject: [PATCH 2/5] docs(no-ticket): give the correct default retry count in the changelog The CLI default for --error-retry-max is 6, not 5. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8363b430..3e187348 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed -- An API request no longer waits without a limit when the API host accepts the connection but does not reply. A request now fails after 30 seconds without data, and the CLI retries it 5 times by default (`--error-retry-max`). A POST request has only the 30-second connection timeout, because a retry after a read timeout can create a second entry. File uploads and downloads do not change. +- An API request no longer waits without a limit when the API host accepts the connection but does not reply. A request now fails after 30 seconds without data, and the CLI retries it 6 times by default (`--error-retry-max`). A POST request has only the 30-second connection timeout, because a retry after a read timeout can create a second entry. File uploads and downloads do not change. ## [1.30.0] - 2026-10-22 From 1867c985d2a3369f1994c9e504c0dcaad4a92e28 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Sat, 24 Oct 2026 18:58:28 +0100 Subject: [PATCH 3/5] fix(no-ticket): stop a stalled POST from waiting forever Give POST and PATCH requests a 120-second read timeout. The API CDN returns a 504 after 60 seconds, so a healthy API replies in time. Do not retry a POST or PATCH request after a read timeout or a dropped connection. The API can have processed the request, so a retry can create a second entry. Connect errors still get retries. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 +- cloudsmith_cli/core/rest.py | 15 ++++---- cloudsmith_cli/core/session.py | 15 ++++++++ cloudsmith_cli/core/tests/test_rest.py | 49 +++++++++++++++++++++++--- 4 files changed, 71 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e187348..d19a22bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed -- An API request no longer waits without a limit when the API host accepts the connection but does not reply. A request now fails after 30 seconds without data, and the CLI retries it 6 times by default (`--error-retry-max`). A POST request has only the 30-second connection timeout, because a retry after a read timeout can create a second entry. File uploads and downloads do not change. +- An API request no longer waits without a limit when the API host accepts the connection but does not reply. A request now fails after 30 seconds without data, and the CLI retries it 6 times by default (`--error-retry-max`). File uploads and downloads do not change. +- A POST request to the API no longer waits without a limit when the API does not reply. A POST or PATCH request now fails after 120 seconds without data. The CLI does not retry a POST or PATCH request after a read timeout or a dropped connection, because the API can have processed it. ## [1.30.0] - 2026-10-22 diff --git a/cloudsmith_cli/core/rest.py b/cloudsmith_cli/core/rest.py index 99992922..98b3fc36 100644 --- a/cloudsmith_cli/core/rest.py +++ b/cloudsmith_cli/core/rest.py @@ -12,11 +12,13 @@ import requests.exceptions from cloudsmith_api.rest import ApiException, RESTClientObject -from .session import create_requests_session +from .session import NON_IDEMPOTENT_METHODS, create_requests_session logger = logging.getLogger(__name__) REQUEST_TIMEOUT_SECONDS = 30 +# The API CDN returns a 504 after 60 seconds without a reply from the API. +NON_IDEMPOTENT_READ_TIMEOUT_SECONDS = 120 Timeout = float | tuple[float | None, float | None] @@ -28,10 +30,10 @@ def _is_seconds(value: object) -> bool: def _resolve_timeout(method: str, timeout: Timeout | None) -> Timeout: """Return the timeout for a request, or the default when there is none.""" if timeout is None: - # A retry after a read timeout sends the request again, so a POST gets - # only a connect timeout and cannot create a second entry. - if method == "POST": - return (REQUEST_TIMEOUT_SECONDS, None) + # The session does not retry a read timeout for these methods, so + # a slow but healthy request gets more time. + if method in NON_IDEMPOTENT_METHODS: + return (REQUEST_TIMEOUT_SECONDS, NON_IDEMPOTENT_READ_TIMEOUT_SECONDS) return REQUEST_TIMEOUT_SECONDS if _is_seconds(timeout): return timeout @@ -107,7 +109,8 @@ def request( reading/decoding response data. Default is True. :param _request_timeout: timeout setting for this request. If one number provided, it will be total request timeout. It can also be a pair (tuple) of (connection, read) timeouts. - Without it, the request uses REQUEST_TIMEOUT_SECONDS, but a POST gets no read timeout. + Without it, the request uses REQUEST_TIMEOUT_SECONDS, and a POST or PATCH + waits NON_IDEMPOTENT_READ_TIMEOUT_SECONDS for a reply. """ # Based on the RESTClientObject class generated by Swagger method = method.upper() diff --git a/cloudsmith_cli/core/session.py b/cloudsmith_cli/core/session.py index 341c60cf..ea51092f 100644 --- a/cloudsmith_cli/core/session.py +++ b/cloudsmith_cli/core/session.py @@ -28,6 +28,9 @@ def get_default_proxy(): return _default_proxy["url"] +NON_IDEMPOTENT_METHODS = frozenset({"POST", "PATCH"}) + + class ApiKeySafeSession(requests.Session): """A session that drops the API key on a redirect to another host.""" @@ -69,8 +72,20 @@ def increment( # A failed certificate check fails the same way on every attempt. if error is not None and find_certificate_error(error) is not None: raise MaxRetryError(_pool, url, error) from error + if self._can_repeat_processed_request(method, error): + raise error return super().increment(method, url, response, error, _pool, _stacktrace) + def _can_repeat_processed_request( + self, method: str | None, error: Exception | None + ) -> bool: + """Return True when a retry can send a request that the server processed.""" + return ( + isinstance(method, str) + and method.upper() in NON_IDEMPOTENT_METHODS + and self._is_read_error(error) + ) + def sleep_for_retry(self, response=None): retry_after = self.get_retry_after(response) if retry_after: diff --git a/cloudsmith_cli/core/tests/test_rest.py b/cloudsmith_cli/core/tests/test_rest.py index b17646d4..6e0e67da 100644 --- a/cloudsmith_cli/core/tests/test_rest.py +++ b/cloudsmith_cli/core/tests/test_rest.py @@ -1,4 +1,6 @@ import ssl +import threading +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer import httpretty import pytest @@ -7,7 +9,11 @@ from urllib3.exceptions import MaxRetryError, SSLError from ..api.init import initialise_api -from ..rest import REQUEST_TIMEOUT_SECONDS, RestClient +from ..rest import ( + NON_IDEMPOTENT_READ_TIMEOUT_SECONDS, + REQUEST_TIMEOUT_SECONDS, + RestClient, +) from ..session import create_requests_session @@ -24,6 +30,28 @@ def patch_httpretty_socket(monkeypatch): ) +@pytest.fixture +def silent_api(): + """Start an API server that reads each request and never replies.""" + requests_seen = [] + release = threading.Event() + + class SilentHandler(BaseHTTPRequestHandler): + def _read_and_stall(self): + self.rfile.read(int(self.headers.get("Content-Length", 0))) + requests_seen.append(self.command) + release.wait() + + do_GET = do_POST = _read_and_stall + + server = ThreadingHTTPServer(("127.0.0.1", 0), SilentHandler) + threading.Thread(target=server.serve_forever, daemon=True).start() + yield f"http://127.0.0.1:{server.server_port}/", requests_seen + release.set() + server.shutdown() + server.server_close() + + class TestRestClient: @httpretty.activate(allow_net_connect=False, verbose=True) def test_implicit_retry_for_status_codes(self): @@ -74,15 +102,14 @@ def _record_timeout(*args, **kwargs): "method, expected_timeout", [ ("GET", REQUEST_TIMEOUT_SECONDS), - ("POST", (REQUEST_TIMEOUT_SECONDS, None)), + ("POST", (REQUEST_TIMEOUT_SECONDS, NON_IDEMPOTENT_READ_TIMEOUT_SECONDS)), ], ) @httpretty.activate(allow_net_connect=False) def test_sets_a_default_timeout(self, monkeypatch, method, expected_timeout): """A stalled API does not block the CLI forever. - A POST gets no read timeout, because the retry after a read timeout - sends the POST again and can create a second entry. + A POST gets a longer read timeout, because the CLI does not retry it. """ initialise_api() client = RestClient() @@ -113,6 +140,20 @@ def test_rejects_an_invalid_timeout(self): assert raised.value.status == 0 + @pytest.mark.parametrize("method, expected_requests", [("POST", 1), ("GET", 3)]) + def test_retries_a_read_timeout_only_for_idempotent_methods( + self, silent_api, method, expected_requests + ): + """A retry after a read timeout can repeat a POST that the server processed.""" + url, requests_seen = silent_api + initialise_api() + client = RestClient(retries=2, backoff_factor=0) + + with pytest.raises(ApiException): + client.request(method, url, _request_timeout=(1, 0.2)) + + assert len(requests_seen) == expected_requests + class TestCreateRequestsSession: @pytest.fixture(autouse=True) From aa2eb2823e9ebe9a0f94619439562fee98664ee4 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Sat, 24 Oct 2026 19:04:26 +0100 Subject: [PATCH 4/5] fix(no-ticket): wait longer than the API CDN before a read timeout The API CDN returns a 504 after 60 seconds. A 30-second read timeout stopped a slow request that was healthy, and the session then sent it again. The default read timeout is now 90 seconds. The connection timeout stays at 30 seconds. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- cloudsmith_cli/core/rest.py | 7 +++++-- cloudsmith_cli/core/tests/test_rest.py | 4 ++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e187348..22934bb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed -- An API request no longer waits without a limit when the API host accepts the connection but does not reply. A request now fails after 30 seconds without data, and the CLI retries it 6 times by default (`--error-retry-max`). A POST request has only the 30-second connection timeout, because a retry after a read timeout can create a second entry. File uploads and downloads do not change. +- An API request no longer waits without a limit when the API host accepts the connection but does not reply. A request now fails when it cannot connect in 30 seconds, or when it gets no data for 90 seconds. The API CDN waits 60 seconds for the API, so the CLI does not stop a slow request before the CDN does. If a request fails, the CLI retries it up to 6 times by default (`--error-retry-max`). A POST request has only the 30-second connection timeout, because a retry after a read timeout can create a second entry. File uploads and downloads do not change. ## [1.30.0] - 2026-10-22 diff --git a/cloudsmith_cli/core/rest.py b/cloudsmith_cli/core/rest.py index 99992922..f436b8a6 100644 --- a/cloudsmith_cli/core/rest.py +++ b/cloudsmith_cli/core/rest.py @@ -16,6 +16,8 @@ logger = logging.getLogger(__name__) +# The API CDN returns a 504 after 60 seconds, so the client waits longer than that. +READ_TIMEOUT_SECONDS = 90 REQUEST_TIMEOUT_SECONDS = 30 Timeout = float | tuple[float | None, float | None] @@ -32,7 +34,7 @@ def _resolve_timeout(method: str, timeout: Timeout | None) -> Timeout: # only a connect timeout and cannot create a second entry. if method == "POST": return (REQUEST_TIMEOUT_SECONDS, None) - return REQUEST_TIMEOUT_SECONDS + return (REQUEST_TIMEOUT_SECONDS, READ_TIMEOUT_SECONDS) if _is_seconds(timeout): return timeout if ( @@ -107,7 +109,8 @@ def request( reading/decoding response data. Default is True. :param _request_timeout: timeout setting for this request. If one number provided, it will be total request timeout. It can also be a pair (tuple) of (connection, read) timeouts. - Without it, the request uses REQUEST_TIMEOUT_SECONDS, but a POST gets no read timeout. + Without it, the request uses REQUEST_TIMEOUT_SECONDS to connect and READ_TIMEOUT_SECONDS + to read, but a POST gets no read timeout. """ # Based on the RESTClientObject class generated by Swagger method = method.upper() diff --git a/cloudsmith_cli/core/tests/test_rest.py b/cloudsmith_cli/core/tests/test_rest.py index b17646d4..64ae9b3c 100644 --- a/cloudsmith_cli/core/tests/test_rest.py +++ b/cloudsmith_cli/core/tests/test_rest.py @@ -7,7 +7,7 @@ from urllib3.exceptions import MaxRetryError, SSLError from ..api.init import initialise_api -from ..rest import REQUEST_TIMEOUT_SECONDS, RestClient +from ..rest import READ_TIMEOUT_SECONDS, REQUEST_TIMEOUT_SECONDS, RestClient from ..session import create_requests_session @@ -73,7 +73,7 @@ def _record_timeout(*args, **kwargs): @pytest.mark.parametrize( "method, expected_timeout", [ - ("GET", REQUEST_TIMEOUT_SECONDS), + ("GET", (REQUEST_TIMEOUT_SECONDS, READ_TIMEOUT_SECONDS)), ("POST", (REQUEST_TIMEOUT_SECONDS, None)), ], ) From a254e6d769b011c239f4c7189136019e9b3e4d2a Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Tue, 3 Nov 2026 10:43:11 +0000 Subject: [PATCH 5/5] Test that POST and PATCH do not repeat a lost reply Check that a dropped connection and a read timeout cause one attempt for POST and PATCH, and retries for GET. Check that POST and PATCH still retry connect errors and retryable statuses. Scope the changelog to unchanged upload and download timeouts. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 +- cloudsmith_cli/core/tests/test_rest.py | 56 ++++++++++++++++++++++++-- 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e191472..66f7ce60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,7 +42,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - `cloudsmith mcp start`, `list_tools`, and `list_groups` now show the standard API error when the CLI cannot load the API schema. An API key that is not valid is one cause. The error includes the API detail and a hint, and `-F json` prints the standard JSON error. Previously, the commands printed an `HTTPStatusError` and sent an error report. `cloudsmith mcp start` writes the error to stderr, so the MCP client does not get it on stdout. - When the API or a proxy answers the SAML IdP request with an HTML page or a JSON value that is not an object, `cloudsmith auth` now shows "Failed to authenticate via SAML!" with the reason. Before this fix, the CLI stopped with a `JSONDecodeError` or an `AttributeError`. The 2FA code exchange and the SAML token refresh now also reject a response that is not a JSON object. The 2FA code exchange also rejects a response without an access token. - List commands that return no results now print `Results: 0 `, not `Results: 1-0 (0) of 0 (page: 1/1, ...)`. When the API response has no pagination headers, list commands print only the result count, not `of None`. -- An API request no longer waits without a limit when the API host accepts the connection but does not reply. A request now fails when it cannot connect in 30 seconds, or when it gets no data for 90 seconds. The API CDN waits 60 seconds for the API, so the CLI does not stop a slow request before the CDN does. If a request fails, the CLI retries it up to 6 times by default (`--error-retry-max`). File uploads and downloads do not change. +- An API request no longer waits without a limit when the API host accepts the connection but does not reply. A request now fails when it cannot connect in 30 seconds, or when it gets no data for 90 seconds. The API CDN waits 60 seconds for the API, so the CLI does not stop a slow request before the CDN does. If a request fails, the CLI retries it up to 6 times by default (`--error-retry-max`). The timeouts for file uploads and downloads do not change. - Every command, including `--help`, no longer stops with `TypeError` or `AttributeError` about `ColorMode` when the installed `click` is older than 8.2. The CLI needs `click` 8.2 or later, but the install of another package can replace it with an older `click`. - `cloudsmith credential-helper install cargo` no longer replaces a `credential-provider` that you set on a Cloudsmith registry in `config.toml`. The install keeps your provider and shows a warning that names the registry. Previously, the install replaced your provider, and `uninstall cargo` then removed it. - `cloudsmith credential-helper uninstall pnpm` now removes the `pnpm-credential-cloudsmith` launcher when the `.npmrc` file does not exist. Previously, it printed `.npmrc file doesn't exist, nothing to do` and did not remove the launcher. The command does not create the `.npmrc` file. @@ -50,7 +50,7 @@ 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. -- A POST request to the API no longer waits without a limit when the API does not reply. A POST or PATCH request now fails after 120 seconds without data. The CLI does not retry a POST or PATCH request after a read timeout or a dropped connection, because the API can have processed it. +- A POST request to the API no longer waits without a limit when the API does not reply. A POST or PATCH request now fails after 120 seconds without data. The CLI does not retry a POST or PATCH request after a read timeout or a dropped connection, because the API can have processed it. This also applies to the POST request that uploads a file. ## [1.30.0] - 2026-10-22 diff --git a/cloudsmith_cli/core/tests/test_rest.py b/cloudsmith_cli/core/tests/test_rest.py index 6fa79c56..c5e497f6 100644 --- a/cloudsmith_cli/core/tests/test_rest.py +++ b/cloudsmith_cli/core/tests/test_rest.py @@ -6,6 +6,7 @@ import httpretty import pytest import requests +import urllib3 from cloudsmith_api.rest import ApiException from urllib3.exceptions import MaxRetryError, NameResolutionError, ProxyError, SSLError @@ -42,9 +43,12 @@ class SilentHandler(BaseHTTPRequestHandler): def _read_and_stall(self): self.rfile.read(int(self.headers.get("Content-Length", 0))) requests_seen.append(self.command) + if self.path == "/drop": + self.close_connection = True + return release.wait() - do_GET = do_POST = _read_and_stall + do_GET = do_POST = do_PATCH = _read_and_stall server = ThreadingHTTPServer(("127.0.0.1", 0), SilentHandler) threading.Thread(target=server.serve_forever, daemon=True).start() @@ -55,8 +59,9 @@ def _read_and_stall(self): class TestRestClient: + @pytest.mark.parametrize("method", ["GET", "POST", "PATCH"]) @httpretty.activate(allow_net_connect=False, verbose=True) - def test_implicit_retry_for_status_codes(self): + def test_implicit_retry_for_status_codes(self, method): """Assert that the rest client retries certain status codes automatically.""" # initialise_api() needs to be called before RestClient can be instantiated, # because it adds default attributes to cloudsmith_api.Configuration which @@ -67,7 +72,6 @@ def test_implicit_retry_for_status_codes(self): client = RestClient(backoff_factor=0) - method = "GET" url = "https://test.site" httpretty.register_uri( @@ -142,7 +146,9 @@ def test_rejects_an_invalid_timeout(self): assert raised.value.status == 0 - @pytest.mark.parametrize("method, expected_requests", [("POST", 1), ("GET", 3)]) + @pytest.mark.parametrize( + "method, expected_requests", [("POST", 1), ("PATCH", 1), ("GET", 3)] + ) def test_retries_a_read_timeout_only_for_idempotent_methods( self, silent_api, method, expected_requests ): @@ -156,6 +162,48 @@ def test_retries_a_read_timeout_only_for_idempotent_methods( assert len(requests_seen) == expected_requests + @pytest.mark.parametrize( + "method, expected_requests", [("POST", 1), ("PATCH", 1), ("GET", 3)] + ) + def test_retries_a_dropped_connection_only_for_idempotent_methods( + self, silent_api, method, expected_requests + ): + """The server can drop the connection after it processed the request.""" + url, requests_seen = silent_api + initialise_api() + client = RestClient(retries=2, backoff_factor=0) + + with pytest.raises(ApiException): + client.request(method, f"{url}drop", body={}, _request_timeout=(1, 5)) + + assert len(requests_seen) == expected_requests + + @pytest.mark.parametrize("method", ["POST", "PATCH"]) + def test_retries_a_connect_error_for_non_idempotent_methods( + self, monkeypatch, method + ): + """The server cannot process a request that did not connect.""" + with socket.socket() as unused_socket: + unused_socket.bind(("127.0.0.1", 0)) + closed_port = unused_socket.getsockname()[1] + connect_attempts = [] + original_connect = urllib3.connection.HTTPConnection.connect + + def _count_connect(connection): + connect_attempts.append(connection.port) + return original_connect(connection) + + monkeypatch.setattr( + urllib3.connection.HTTPConnection, "connect", _count_connect + ) + initialise_api() + client = RestClient(retries=2, backoff_factor=0) + + with pytest.raises(ApiException): + client.request(method, f"http://127.0.0.1:{closed_port}/", body={}) + + assert len(connect_attempts) == 3 + class TestCreateRequestsSession: @pytest.fixture(autouse=True)