fix(no-ticket): stop a stalled POST from waiting forever

CHANGELOG.md

@@ -43,14 +43,15 @@ 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 <items>`, not `Results: 1-0 (0) of 0 <items> (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`). 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`). 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.
 - `cloudsmith credential-helper install` and `uninstall` no longer replace a Docker `config.json` or a Cargo `config.toml` that does not parse. Previously, the CLI read such a file as empty and wrote over it, so your settings were lost. Now the command stops with an error that names the file, and it does not write to the file. Correct or remove the file. Then run the command again.
 - `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. This also applies to the POST request that uploads a file.
 - `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.
 
 ## [1.30.0] - 2026-10-22

cloudsmith_cli/core/rest.py

@@ -12,13 +12,15 @@
 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__)
 
 # The API CDN returns a 504 after 60 seconds, so the client waits longer than that.
 READ_TIMEOUT_SECONDS = 90
 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]
 
@@ -30,10 +32,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, READ_TIMEOUT_SECONDS)
     if _is_seconds(timeout):
         return timeout
@@ -110,7 +112,7 @@ def request(
         :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 to connect and READ_TIMEOUT_SECONDS
-                                 to read, but a POST gets no read timeout.
+                                 to read, but a POST or PATCH waits NON_IDEMPOTENT_READ_TIMEOUT_SECONDS for a reply.
         """
         # Based on the RESTClientObject class generated by Swagger
         method = method.upper()

cloudsmith_cli/core/session.py

@@ -39,6 +39,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."""
 
@@ -94,8 +97,20 @@ def increment(
             find_certificate_error(error) is not None or _is_unknown_host_error(error)
         ):
             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:

cloudsmith_cli/core/tests/test_rest.py

@@ -1,14 +1,22 @@
 import socket
 import ssl
+import threading
+from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer
 
 import httpretty
 import pytest
 import requests
+import urllib3
 from cloudsmith_api.rest import ApiException
 from urllib3.exceptions import MaxRetryError, NameResolutionError, ProxyError, SSLError
 
 from ..api.init import initialise_api
-from ..rest import READ_TIMEOUT_SECONDS, REQUEST_TIMEOUT_SECONDS, RestClient
+from ..rest import (
+    NON_IDEMPOTENT_READ_TIMEOUT_SECONDS,
+    READ_TIMEOUT_SECONDS,
+    REQUEST_TIMEOUT_SECONDS,
+    RestClient,
+)
 from ..session import create_requests_session
 
 
@@ -25,9 +33,35 @@
     )
 
 
+@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)
+            if self.path == "/drop":
+                self.close_connection = True
+                return
+            release.wait()
+
+        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()
+    yield f"http://127.0.0.1:{server.server_port}/", requests_seen
+    release.set()
+    server.shutdown()
+    server.server_close()
+
+
 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
@@ -38,7 +72,6 @@
 
         client = RestClient(backoff_factor=0)
 
-        method = "GET"
         url = "https://test.site"
 
         httpretty.register_uri(
@@ -75,15 +108,14 @@
         "method, expected_timeout",
         [
             ("GET", (REQUEST_TIMEOUT_SECONDS, READ_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()
@@ -114,6 +146,64 @@
 
         assert raised.value.status == 0
 
+    @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
+    ):
+        """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
+
+    @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)