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 <maxim.shulga@alludo.com>

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.

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
 

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",

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."""

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