From f724151467619101ea75ca9d637ece7b89962e10 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Sat, 24 Oct 2026 19:14:26 +0100 Subject: [PATCH] fix(no-ticket): do not retry a POST after a 500, 502 or 504 response A gateway can send these codes after the API processed the request. A retry then sends the POST again and can create a second entry. The CLI still retries a 429 or 503 response. A file upload keeps its retries, because a second upload of the same file is safe. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + cloudsmith_cli/cli/decorators.py | 4 ++- cloudsmith_cli/core/api/files.py | 3 +- cloudsmith_cli/core/session.py | 36 +++++++++++++++++++---- cloudsmith_cli/core/tests/test_files.py | 39 +++++++++++++++++++++++++ cloudsmith_cli/core/tests/test_rest.py | 19 ++++++++++++ 6 files changed, 95 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d19a22bd..543c0cff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - 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. +- The CLI no longer sends a POST or PATCH request to the API again after a 500, 502 or 504 response. A gateway can send these codes after the API created the entry, so a retry could create a second entitlement, token or package. The CLI still retries a 429 or 503 response, and it ignores 500, 502 and 504 in `--error-retry-codes` for these requests. A file upload still retries these codes, a read timeout and a dropped connection, because a second upload of the same file is safe. ## [1.30.0] - 2026-10-22 diff --git a/cloudsmith_cli/cli/decorators.py b/cloudsmith_cli/cli/decorators.py index 9c45d4dc..ab72a57f 100644 --- a/cloudsmith_cli/cli/decorators.py +++ b/cloudsmith_cli/cli/decorators.py @@ -781,7 +781,9 @@ def initialise_api(f): default="429,500,502,503,504", help="The status codes that when received from the API will cause " "a retry (if --error-retry-max is > 0). By default this will be for " - "429, 500, 502, 503 and 504 error codes.", + "429, 500, 502, 503 and 504 error codes. The CLI does not retry a POST " + "or PATCH request after a 500, 502 or 504 code, even if you list it, " + "because the API can have processed the request.", ) @click.pass_context @functools.wraps(f) diff --git a/cloudsmith_cli/core/api/files.py b/cloudsmith_cli/core/api/files.py index d8580972..241832f7 100644 --- a/cloudsmith_cli/core/api/files.py +++ b/cloudsmith_cli/core/api/files.py @@ -91,7 +91,8 @@ def upload_file(upload_url, upload_fields, filepath, callback=None): client = get_files_api() headers["user-agent"] = client.api_client.user_agent - session = create_requests_session() + # A second upload of the same file replaces the first one. + session = create_requests_session(repeat_non_idempotent=True) resp = _send_upload_request( session.post, upload_url, data=monitor, headers=headers, proxies=proxies ) diff --git a/cloudsmith_cli/core/session.py b/cloudsmith_cli/core/session.py index ea51092f..1eef581f 100644 --- a/cloudsmith_cli/core/session.py +++ b/cloudsmith_cli/core/session.py @@ -3,6 +3,7 @@ import ssl import sys import time +from typing import Any import requests import urllib3 @@ -29,6 +30,8 @@ def get_default_proxy(): NON_IDEMPOTENT_METHODS = frozenset({"POST", "PATCH"}) +# A gateway can send these after the API processed the request. +PROCESSED_REQUEST_STATUS_CODES = frozenset({500, 502, 504}) class ApiKeySafeSession(requests.Session): @@ -52,14 +55,26 @@ def merge_environment_settings(self, url, proxies, stream, verify, cert): class RetryWithCallback(Retry): """A urllib3 Retry with a callback on retries.""" - def __init__(self, *args, **kwargs): + def __init__(self, *args: Any, **kwargs: Any) -> None: self.error_retry_cb = kwargs.pop("error_retry_cb", None) + self.repeat_non_idempotent = bool(kwargs.pop("repeat_non_idempotent", False)) super().__init__(*args, **kwargs) - def new(self, **kw): + def new(self, **kw: Any) -> "RetryWithCallback": kw["error_retry_cb"] = self.error_retry_cb + kw["repeat_non_idempotent"] = self.repeat_non_idempotent return super().new(**kw) + def is_retry( + self, method: str, status_code: int, has_retry_after: bool = False + ) -> bool: + if ( + self._is_unsafe_to_repeat(method) + and status_code in PROCESSED_REQUEST_STATUS_CODES + ): + return False + return super().is_retry(method, status_code, has_retry_after) + def increment( self, method=None, @@ -80,10 +95,14 @@ 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 self._is_unsafe_to_repeat(method) and self._is_read_error(error) + + def _is_unsafe_to_repeat(self, method: str | None) -> bool: + """Return True when a second send of the request can repeat its effect.""" return ( - isinstance(method, str) + not self.repeat_non_idempotent + and isinstance(method, str) and method.upper() in NON_IDEMPOTENT_METHODS - and self._is_read_error(error) ) def sleep_for_retry(self, response=None): @@ -177,8 +196,14 @@ def create_requests_session( respect_retry_after_header=True, user_agent=None, headers=None, + repeat_non_idempotent: bool = False, ): - """Create a requests session that retries some errors.""" + """Create a requests session that retries some errors. + + A POST or PATCH request is not retried after a read error or a 500, 502 + or 504 response, because the server can have processed it. Set + repeat_non_idempotent when a second send of the request is safe. + """ # pylint: disable=too-many-branches config = _sdk_configuration() @@ -232,6 +257,7 @@ def create_requests_session( total=retries, error_retry_cb=error_retry_cb, respect_retry_after_header=respect_retry_after_header, + repeat_non_idempotent=repeat_non_idempotent, ) adapter = SSLContextAdapter( diff --git a/cloudsmith_cli/core/tests/test_files.py b/cloudsmith_cli/core/tests/test_files.py index 9968be34..5755607d 100644 --- a/cloudsmith_cli/core/tests/test_files.py +++ b/cloudsmith_cli/core/tests/test_files.py @@ -1,7 +1,10 @@ """Tests for the files API upload helpers.""" +from functools import partial from unittest.mock import Mock, patch +import httpretty +import httpretty.core import pytest import requests from cloudsmith_api.rest import ApiException as SdkApiException @@ -9,6 +12,7 @@ from ..api.exceptions import ApiException from ..api.files import multi_part_upload_file, upload_file from ..credentials.models import CredentialResult +from ..session import create_requests_session def upload_and_capture_headers(tmp_path, credential, api_key=None): @@ -117,3 +121,38 @@ def test_failed_multi_part_completion(self, tmp_path): ) assert raised.value.status == 0 + + +@httpretty.activate(allow_net_connect=False) +def test_upload_retries_a_gateway_error(tmp_path, monkeypatch): + """A second upload of the same file is safe, so the CLI retries it.""" + # urllib3 2.x calls shutdown() on a retry, and httpretty does not fake it. + monkeypatch.setattr( + httpretty.core.fakesock.socket, + "shutdown", + lambda self, how: None, + raising=False, + ) + filepath = tmp_path / "package.raw" + filepath.write_bytes(b"payload") + upload_url = "http://upload.example.invalid/" + requests_seen = [] + + def respond(request, uri, response_headers): + requests_seen.append(uri) + return [504, response_headers, ""] + + httpretty.register_uri(httpretty.POST, upload_url, body=respond) + files_api = Mock(**{"api_client.user_agent": "cloudsmith-cli"}) + + with ( + patch( + "cloudsmith_cli.core.api.files.create_requests_session", + partial(create_requests_session, retries=2, backoff_factor=0), + ), + patch("cloudsmith_cli.core.api.files.get_files_api", return_value=files_api), + pytest.raises(ApiException), + ): + upload_file(upload_url, {}, str(filepath)) + + assert len(requests_seen) == 3 diff --git a/cloudsmith_cli/core/tests/test_rest.py b/cloudsmith_cli/core/tests/test_rest.py index 6e0e67da..54853543 100644 --- a/cloudsmith_cli/core/tests/test_rest.py +++ b/cloudsmith_cli/core/tests/test_rest.py @@ -154,6 +154,25 @@ def test_retries_a_read_timeout_only_for_idempotent_methods( assert len(requests_seen) == expected_requests + @pytest.mark.parametrize( + "method, status, expected_requests", + [("POST", 504, 1), ("POST", 503, 3), ("GET", 504, 3)], + ) + @httpretty.activate(allow_net_connect=False) + def test_retries_a_gateway_error_only_for_idempotent_methods( + self, method, status, expected_requests + ): + """A gateway error can replace the reply to a POST that the API processed.""" + initialise_api() + client = RestClient(retries=2, backoff_factor=0) + url = "https://test.site" + httpretty.register_uri(method, url, status=status, body="") + + with pytest.raises(ApiException): + client.request(method, url) + + assert len(httpretty.latest_requests()) == expected_requests + class TestCreateRequestsSession: @pytest.fixture(autouse=True)