diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a0b272df..70c476b25 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/openeo/rest/_connection.py b/openeo/rest/_connection.py index ce521e47e..6858b0985 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: + # 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) diff --git a/tests/rest/test_job.py b/tests/rest/test_job.py index 6e04df082..98ab81ffd 100644 --- a/tests/rest/test_job.py +++ b/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"