Use standard retry logic in _download_ranged

CHANGELOG.md

@@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Convert setup.py to pyproject.toml ([#920](https://github.com/Open-EO/openeo-python-client/issues/920))
 - Make `_DerivedFrom._from_url` more resilient against unresolvable/unparsable `derived_from` links ([#928](https://github.com/Open-EO/openeo-python-client/issues/928), eu-cdse/openeo-cdse-infra#1338)
 - `download_url()`: favor explicit HEAD status check over unhelpful auto-check ([#939](https://github.com/Open-EO/openeo-python-client/issues/939))
+- `_download_ranged` no longer implements its own retry loop; transient failures are retried by the standard urllib3 retry configuration of the connection's session ([#934](https://github.com/Open-EO/openeo-python-client/issues/934))
 
 ### Removed
 

openeo/rest/_connection.py

@@ -26,35 +26,14 @@
     str_truncate,
     url_join,
 )
-from openeo.utils.http import (
-    HTTP_408_REQUEST_TIMEOUT,
-    HTTP_429_TOO_MANY_REQUESTS,
-    HTTP_500_INTERNAL_SERVER_ERROR,
-    HTTP_501_NOT_IMPLEMENTED,
-    HTTP_502_BAD_GATEWAY,
-    HTTP_503_SERVICE_UNAVAILABLE,
-    HTTP_504_GATEWAY_TIMEOUT,
-    session_with_retries,
-)
+from openeo.utils.http import HTTP_502_BAD_GATEWAY, session_with_retries
 
 _log = logging.getLogger(__name__)
 
 # Default timeouts for requests
 # TODO: get default_timeout from config?
 DEFAULT_TIMEOUT = 20 * 60
 
-MAX_DOWNLOAD_RETRIES_PER_RANGE = 3
-
-RETRIABLE_DOWNLOAD_STATUSCODES = [
-    HTTP_408_REQUEST_TIMEOUT,
-    HTTP_429_TOO_MANY_REQUESTS,
-    HTTP_500_INTERNAL_SERVER_ERROR,
-    HTTP_501_NOT_IMPLEMENTED,
-    HTTP_502_BAD_GATEWAY,
-    HTTP_503_SERVICE_UNAVAILABLE,
-    HTTP_504_GATEWAY_TIMEOUT,
-]
-
 
 class RestApiConnection:
     """Base connection class implementing generic REST API request functionality"""
@@ -331,25 +310,15 @@ def _download_ranged(
         chunk_size: int = DEFAULT_DOWNLOAD_CHUNK_SIZE,
         range_size: int = DEFAULT_DOWNLOAD_RANGE_SIZE,
     ) -> None:
+        # No per-range retry loop here: the connection's session is expected
+        # to have urllib3 Retry mounted (as done by default in
+        # openeo.connect), so transient failures are retried there.
         ensure_parent_dir_for(target)
         with target.open("wb") as f:
             for from_byte_index in range(0, file_size, range_size):
                 to_byte_index = min(from_byte_index + range_size - 1, file_size - 1)
-                tries_left = MAX_DOWNLOAD_RETRIES_PER_RANGE
-                while tries_left > 0:
-                    try:
-                        range_headers = {"Range": f"bytes={from_byte_index}-{to_byte_index}"}
-                        with self.get(path=url, headers=range_headers, stream=True) as r:
-                            r.raise_for_status()
-                            for block in r.iter_content(chunk_size=chunk_size):
-                                f.write(block)
-                        break
-                    except OpenEoApiPlainError as error:
-                        tries_left -= 1
-                        if tries_left > 0 and error.http_status_code in RETRIABLE_DOWNLOAD_STATUSCODES:
-                            _log.warning(
-                                f"Failed to retrieve chunk {from_byte_index}-{to_byte_index} from {url} (status {error.http_status_code}) - retrying"
-                            )
-                            continue
-                        else:
-                            raise error
+                range_headers = {"Range": f"bytes={from_byte_index}-{to_byte_index}"}
+                with self.get(path=url, headers=range_headers, stream=True) as r:
+                    r.raise_for_status()
+                    for block in r.iter_content(chunk_size=chunk_size):
+                        f.write(block)

tests/rest/test_job.py

@@ -825,6 +825,49 @@ def test_get_results_download_file_ranged(job_with_chunked_asset_using_head: Bat
         assert f.read() == TIFF_CONTENT
 
 
+@httpretty.activate(allow_net_connect=False)
+def test_download_url_ranged_retries_transient_503(tmp_path):
+    """#934: transient failures during ranged download are retried by the
+    standard urllib3 Retry of the connection's session, not a custom loop."""
+    content = b"A" * 200 + b"B" * 100  # 300 bytes -> two 200-byte ranges
+
+    # httpretty requires the registered HEAD body to match Content-Length;
+    # the body itself is never read back for a HEAD request.
+    httpretty.register_uri(
+        httpretty.HEAD,
+        uri=API_URL + "/dl/ranged.bin",
+        body="X" * 300,
+        adding_headers={"Content-Length": "300", "Accept-Ranges": "bytes"},
+    )
+    httpretty.register_uri(
+        httpretty.GET,
+        uri=API_URL + "/dl/ranged.bin",
+        responses=[
+            # First attempt on range 0-199: transient failure.
+            httpretty.Response(status=503, body="Service Unavailable"),
+            # Retry of range 0-199 succeeds.
+            httpretty.Response(status=206, body="A" * 200, adding_headers={"Content-Range": "bytes 0-199/300"}),
+            # Range 200-299 succeeds on the first attempt.
+            httpretty.Response(status=206, body="B" * 100, adding_headers={"Content-Range": "bytes 200-299/300"}),
+        ],
+    )
+    # /.well-known/openeo is intentionally not mocked: the version discovery
+    # request fails leniently and falls back to the given url, so no fake
+    # discovery document is needed. GET / however is the capabilities request
+    # the connection itself makes, so it needs a minimal valid document.
+    httpretty.register_uri(
+        httpretty.GET,
+        uri=API_URL + "/",
+        body=json.dumps({"api_version": "1.0.0", "endpoints": []}),
+    )
+
+    con = openeo.connect(API_URL)
+    with mock.patch("time.sleep"):
+        con.download_url(API_URL + "/dl/ranged.bin", tmp_path / "ranged.bin", range_size=200)
+
+    assert (tmp_path / "ranged.bin").read_bytes() == content
+
+
 def test_download_result_folder(job_with_1_asset: BatchJob, tmp_path):
     job = job_with_1_asset
     target = tmp_path / "folder"