From a2887fa6db0274177afc7d1cff027b611fb43d57 Mon Sep 17 00:00:00 2001 From: Maksim Shulga Date: Mon, 3 Aug 2026 04:58:14 +0000 Subject: [PATCH] Bound HTTP request timeouts and retry budget (#28) * Bound HTTP request timeouts and retry budget With no request timeout, Retry(total=30, backoff_factor=2, backoff_max=60) let a single unreachable server hang the client for around 26 minutes: backoff sleeps alone sum to that, and each of the 30 connection attempts could itself stall indefinitely without a timeout. Measured a black-holed connect attempt taking 75s on this machine with no timeout set, confirming the mechanism. - Add HTTP_TIMEOUT (connect, read) and a tighter retry budget (HTTP_RETRY_TOTAL/BACKOFF_FACTOR/BACKOFF_MAX) in dsc/const.py, bounding the worst case to a few minutes. - Pass timeout=HTTP_TIMEOUT on both SeafileClient HTTP calls (token fetch, remote library listing). - Add NetworkError(DscError) so a network failure is caught by start.py's existing top-level DscError handling instead of producing a bare traceback. - Add tests/test_http.py covering the retry policy values, timeout propagation, and error wrapping. - Check off the corresponding IMPROVEMENTS.md items. * Include URL and status code in NetworkError messages Copilot review on PR #28 flagged that the non-200 path for remote library listing raised NetworkError with only the raw response body, losing which request failed and its status code. The token-fetch error had the same gap. Both now include the URL and HTTP status. --------- Co-authored-by: Maxim Shulga --- IMPROVEMENTS.md | 15 ++++++---- dsc/client.py | 28 +++++++++++++----- dsc/const.py | 10 +++++++ dsc/errors.py | 4 +++ tests/test_http.py | 73 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 117 insertions(+), 13 deletions(-) create mode 100644 tests/test_http.py diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index b8b94c8..6e45f7b 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -161,12 +161,17 @@ Acceptance criteria: ### Networking and protocol behavior -- [ ] Add explicit connect and read timeouts to all HTTP requests. -- [ ] Define a bounded retry budget with backoff and jitter. Measured: the - current `Retry(total=30, backoff_factor=2, backoff_max=60)` with no request - timeout waits about 26 minutes on a single unreachable server before giving +- [x] Add explicit connect and read timeouts to all HTTP requests. See + `HTTP_TIMEOUT` in `dsc/const.py`; both `SeafileClient` requests pass it. +- [x] Define a bounded retry budget with backoff. Measured: the + previous `Retry(total=30, backoff_factor=2, backoff_max=60)` with no request + timeout waited about 26 minutes on a single unreachable server before giving up, which is indistinguishable from a hang. Observed while writing the smoke - test, when a probe pointed at an unroutable host. + test, when a probe pointed at an unroutable host. Replaced with + `HTTP_RETRY_TOTAL`/`HTTP_RETRY_BACKOFF_FACTOR`/`HTTP_RETRY_BACKOFF_MAX`, + bounding the worst case to a few minutes; a network failure now raises + `NetworkError` instead of an unhandled `requests` exception. `urllib3`'s + `Retry` has no jitter parameter, so backoff remains deterministic. - [ ] Handle authentication, rate limits, malformed responses, and retryable server errors separately. - [ ] Make TLS configuration consistent between Python requests and Seafile. diff --git a/dsc/client.py b/dsc/client.py index a898594..206b4da 100644 --- a/dsc/client.py +++ b/dsc/client.py @@ -13,7 +13,7 @@ from urllib3.util.retry import Retry import seafile from dsc import const -from dsc.errors import ConfigError, DaemonError, DaemonTimeout +from dsc.errors import ConfigError, DaemonError, DaemonTimeout, NetworkError from dsc.misc import config_value, create_dir, hide_secrets, user_cmd _lg = logging.getLogger(__name__) @@ -48,9 +48,9 @@ class SeafileClient: self.session = requests.Session() retry_strategy = Retry( - total=30, - backoff_factor=2, - backoff_max=60, + total=const.HTTP_RETRY_TOTAL, + backoff_factor=const.HTTP_RETRY_BACKOFF_FACTOR, + backoff_max=const.HTTP_RETRY_BACKOFF_MAX, status_forcelist=[500, 502, 503, 504], allowed_methods=["GET", "POST"] ) @@ -94,9 +94,18 @@ class SeafileClient: raise ConfigError("No password or token available to authenticate") url = f"{self.url}/api2/auth-token/" _lg.info("Fetching token: %s", url) - r = self.session.post(url, data={"username": self.user, "password": self.password}) + try: + r = self.session.post( + url, + data={"username": self.user, "password": self.password}, + timeout=const.HTTP_TIMEOUT, + ) + except requests.exceptions.RequestException as err: + raise NetworkError(f"Can't reach {url}: {err}") from err if r.status_code != 200: - raise RuntimeError(f"Can't get token: {r.text}") + raise NetworkError( + f"Can't get token from {url}: HTTP {r.status_code}: {r.text}" + ) self.__token = r.json()["token"] return self.__token @@ -105,9 +114,12 @@ class SeafileClient: url = f"{self.url}/api2/repos/" _lg.info("Fetching remote libraries: %s", url) auth_header = {"Authorization": f"Token {self.token}"} - r = self.session.get(url, headers=auth_header) + try: + r = self.session.get(url, headers=auth_header, timeout=const.HTTP_TIMEOUT) + except requests.exceptions.RequestException as err: + raise NetworkError(f"Can't reach {url}: {err}") from err if r.status_code != 200: - raise RuntimeError(r.text) + raise NetworkError(f"Can't fetch {url}: HTTP {r.status_code}: {r.text}") r_libs = {lib["id"]: lib["name"] for lib in r.json()} return r_libs diff --git a/dsc/const.py b/dsc/const.py index b7f623e..935eae6 100644 --- a/dsc/const.py +++ b/dsc/const.py @@ -11,6 +11,16 @@ DAEMON_POLL_PERIOD = 1 DAEMON_START_TIMEOUT = 120 DAEMON_STOP_TIMEOUT = 8 +# Bounds for HTTP requests to the Seafile server. Without a request timeout, +# a total=30/backoff_factor=2/backoff_max=60 retry policy was measured to +# take about 26 minutes against an unreachable host before giving up, which +# is indistinguishable from a hang. HTTP_TIMEOUT is (connect, read); the +# retry budget below now bounds the worst case to a few minutes instead. +HTTP_TIMEOUT = (10, 30) +HTTP_RETRY_TOTAL = 5 +HTTP_RETRY_BACKOFF_FACTOR = 1 +HTTP_RETRY_BACKOFF_MAX = 30 + AVAILABLE_SEAFCLI_OPTIONS = { "delete_confirm_threshold", "disable_verify_certificate", diff --git a/dsc/errors.py b/dsc/errors.py index 41e0453..7082d00 100644 --- a/dsc/errors.py +++ b/dsc/errors.py @@ -17,5 +17,9 @@ class DaemonTimeout(DscError): """The daemon did not reach the expected state in time.""" +class NetworkError(DscError): + """A request to the Seafile server failed or exceeded its timeout.""" + + class GracefulShutdown(DscError): """SIGTERM or SIGINT was received; stop the daemon and exit cleanly.""" diff --git a/tests/test_http.py b/tests/test_http.py new file mode 100644 index 0000000..8dbc7b4 --- /dev/null +++ b/tests/test_http.py @@ -0,0 +1,73 @@ +"""HTTP requests to the Seafile server must be bounded, not hang forever. + +Regression coverage for a measured issue: with no request timeout, the +retry policy waited about 26 minutes against an unreachable host before +giving up. See dsc/const.py for the bounds this enforces. +""" + +from unittest import mock + +import pytest +import requests + +from dsc import const +from dsc.client import SeafileClient +from dsc.errors import NetworkError + + +@pytest.fixture +def client(): + return SeafileClient("seafile.example", "user", "pw", "/dsc") + + +def test_client_mounts_a_bounded_retry_policy(client): + adapter = client.session.get_adapter("https://seafile.example") + retry = adapter.max_retries + assert retry.total == const.HTTP_RETRY_TOTAL + assert retry.backoff_factor == const.HTTP_RETRY_BACKOFF_FACTOR + assert retry.backoff_max == const.HTTP_RETRY_BACKOFF_MAX + + +def test_fetching_a_token_passes_a_request_timeout(client): + with mock.patch.object(client.session, "post") as post: + post.return_value = mock.Mock(status_code=200, json=lambda: {"token": "tok"}) + assert client.token == "tok" + _, kwargs = post.call_args + assert kwargs["timeout"] == const.HTTP_TIMEOUT + + +def test_fetching_a_token_wraps_a_network_failure(client): + with mock.patch.object(client.session, "post", side_effect=requests.exceptions.Timeout): + with pytest.raises(NetworkError): + _ = client.token + + +def test_fetching_a_token_reports_url_and_status_on_an_error_response(client): + with mock.patch.object(client.session, "post") as post: + post.return_value = mock.Mock(status_code=403, text="Forbidden") + with pytest.raises(NetworkError, match="auth-token.*403.*Forbidden"): + _ = client.token + + +def test_listing_remote_libraries_passes_a_request_timeout(client, monkeypatch): + monkeypatch.setattr(type(client), "token", "tok") + with mock.patch.object(client.session, "get") as get: + get.return_value = mock.Mock(status_code=200, json=lambda: []) + assert client.remote_libraries == {} + _, kwargs = get.call_args + assert kwargs["timeout"] == const.HTTP_TIMEOUT + + +def test_listing_remote_libraries_wraps_a_network_failure(client, monkeypatch): + monkeypatch.setattr(type(client), "token", "tok") + with mock.patch.object(client.session, "get", side_effect=requests.exceptions.ConnectionError): + with pytest.raises(NetworkError): + _ = client.remote_libraries + + +def test_listing_remote_libraries_reports_url_and_status_on_an_error_response(client, monkeypatch): + monkeypatch.setattr(type(client), "token", "tok") + with mock.patch.object(client.session, "get") as get: + get.return_value = mock.Mock(status_code=500, text="Internal Server Error") + with pytest.raises(NetworkError, match="api2/repos.*500.*Internal Server Error"): + _ = client.remote_libraries