From 1e5531df869f65aeae423b6a2cebb3dc3b01f335 Mon Sep 17 00:00:00 2001 From: niukanen1 <57656076+niukanen1@users.noreply.github.com> Date: Thu, 17 Sep 2026 12:22:15 +0400 Subject: [PATCH] Use standard retry logic in _download_ranged Remove the hand-rolled per-range retry loop (MAX_DOWNLOAD_RETRIES_PER_RANGE + RETRIABLE_DOWNLOAD_STATUSCODES) from _download_ranged. Transient failures are already retried by the urllib3 Retry configuration mounted on the connection's session (session_with_retries), so the DIY loop only added divergent behavior: different retry counts, different status codes (408/500/501 were retried here but not by the session), and OpenEoApiPlainError raised where the session raises RetryError. Fixes #934 --- CHANGELOG.md | 1 + openeo/rest/_connection.py | 49 ++++---------------- tests/rest/test_download_ranged_retry.py | 57 ++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 40 deletions(-) create mode 100644 tests/rest/test_download_ranged_retry.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a7f59c8b..55db89b23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- `_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)) - 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) diff --git a/openeo/rest/_connection.py b/openeo/rest/_connection.py index 6c17c3693..c41e907a3 100644 --- a/openeo/rest/_connection.py +++ b/openeo/rest/_connection.py @@ -26,16 +26,7 @@ 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__) @@ -43,18 +34,6 @@ # 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: + # Retries on transient failures (429/502/503/504) are handled by the + # urllib3 Retry mounted on this connection's session (see + # session_with_retries), so no per-range retry loop is needed here. 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) diff --git a/tests/rest/test_download_ranged_retry.py b/tests/rest/test_download_ranged_retry.py new file mode 100644 index 000000000..a270572b6 --- /dev/null +++ b/tests/rest/test_download_ranged_retry.py @@ -0,0 +1,57 @@ +from pathlib import Path +from unittest import mock + +import httpretty +import pytest + +from openeo.rest._connection import RestApiConnection +from openeo.utils.http import session_with_retries + + +class TestDownloadRangedStandardRetry: + """ + Regression test for #934: `_download_ranged` no longer implements its own + retry loop; transient failures are retried by the standard urllib3 Retry + configuration of the connection's session. + """ + + @pytest.fixture(autouse=True) + def _auto_httpretty_enabled(self): + with httpretty.enabled(allow_net_connect=False): + yield + + def test_transient_503_on_range_is_retried_by_session(self, tmp_path, time_sleep): + content = b"0123456789" * 100 # 1000 bytes -> two 500-byte ranges + url = "https://example.test/dl/file.bin" + + httpretty.register_uri( + httpretty.GET, + uri=url, + responses=[ + # First attempt on range 0-499: transient failure. + httpretty.Response(status=503, body="Service Unavailable"), + # Retry of range 0-499 succeeds. + httpretty.Response( + status=206, + body=content[0:500], + adding_headers={"Content-Range": f"bytes 0-499/{len(content)}"}, + ), + # Range 500-999 succeeds on the first attempt. + httpretty.Response( + status=206, + body=content[500:], + adding_headers={"Content-Range": f"bytes 500-999/{len(content)}"}, + ), + ], + ) + + connection = RestApiConnection(root_url="https://example.test", session=session_with_retries()) + target = tmp_path / "file.bin" + connection._download_ranged(url=url, target=target, file_size=len(content), range_size=500) + + assert target.read_bytes() == content + + @pytest.fixture + def time_sleep(self): + with mock.patch("time.sleep") as m: + yield m