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 1/3] 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 From d6a57c6dd96729c1a2aa0e19fe0bbcf1d441f5d6 Mon Sep 17 00:00:00 2001 From: niukanen1 <57656076+niukanen1@users.noreply.github.com> Date: Mon, 21 Sep 2026 20:57:09 +0400 Subject: [PATCH 2/3] Address review: move retry test to test_job.py, use download_url - the retry regression test lives in test_job.py next to the other ranged download tests, and drives the public download_url method - the test content uses distinct chunks so each 206 range is unique - changelog entry moved to the bottom of the Changed section --- CHANGELOG.md | 2 +- tests/rest/test_download_ranged_retry.py | 57 ------------------------ tests/rest/test_job.py | 53 ++++++++++++++++++++++ 3 files changed, 54 insertions(+), 58 deletions(-) delete mode 100644 tests/rest/test_download_ranged_retry.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 55db89b23..6128cf653 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,9 +11,9 @@ 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) +- `_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/tests/rest/test_download_ranged_retry.py b/tests/rest/test_download_ranged_retry.py deleted file mode 100644 index a270572b6..000000000 --- a/tests/rest/test_download_ranged_retry.py +++ /dev/null @@ -1,57 +0,0 @@ -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 diff --git a/tests/rest/test_job.py b/tests/rest/test_job.py index ed0784334..8f3309b72 100644 --- a/tests/rest/test_job.py +++ b/tests/rest/test_job.py @@ -821,6 +821,59 @@ 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"R4ng3d-D4t4" * 32 # 352 bytes -> one full and one partial 256-byte range + assert len(set(content[i : i + 256] for i in range(0, len(content), 256))) == 2 + + # HEAD body is ignored, but httpretty validates registered Content-Length against it. + httpretty.register_uri( + httpretty.HEAD, + uri=API_URL + "/dl/ranged.bin", + body=content.decode("latin-1"), + adding_headers={"Content-Length": f"{len(content)}", "Accept-Ranges": "bytes"}, + ) + httpretty.register_uri( + httpretty.GET, + uri=API_URL + "/dl/ranged.bin", + responses=[ + # First attempt on range 0-255: transient failure. + httpretty.Response(status=503, body="Service Unavailable"), + # Retry of range 0-255 succeeds. + httpretty.Response( + status=206, + body=content[0:256].decode("latin-1"), + adding_headers={"Content-Range": f"bytes 0-255/{len(content)}"}, + ), + # Range 256-351 succeeds on the first attempt. + httpretty.Response( + status=206, + body=content[256:].decode("latin-1"), + adding_headers={"Content-Range": f"bytes 256-351/{len(content)}"}, + ), + ], + ) + + httpretty.register_uri( + httpretty.GET, + uri=API_URL + "/.well-known/openeo", + body=json.dumps({"api_version": "1.0.0", "endpoints": [{"path": "/credentials/basic", "methods": ["GET"]}]}), + ) + httpretty.register_uri( + httpretty.GET, + uri=API_URL + "/", + body=json.dumps({"api_version": "1.0.0", "endpoints": [{"path": "/credentials/basic", "methods": ["GET"]}]}), + ) + + con = openeo.connect(API_URL) + with mock.patch("time.sleep"): + con.download_url(API_URL + "/dl/ranged.bin", tmp_path / "ranged.bin", range_size=256) + + 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" From 690e3d357dd86c80f23c086c052fd45e1bc643ff Mon Sep 17 00:00:00 2001 From: niukanen1 <57656076+niukanen1@users.noreply.github.com> Date: Fri, 25 Sep 2026 11:27:16 +0400 Subject: [PATCH 3/3] Address review: simplify ranged-retry test mocks and fix comment - drop the /.well-known/openeo mock: version discovery fails leniently - use plain 'A'*200 + 'B'*100 content instead of the chunk-uniqueness juggling - document why the HEAD registration still needs a body (httpretty constraint) - reword the _download_ranged comment: retry behavior depends on the session setup, which is not guaranteed at this point --- openeo/rest/_connection.py | 6 +++--- tests/rest/test_job.py | 42 +++++++++++++++----------------------- 2 files changed, 19 insertions(+), 29 deletions(-) diff --git a/openeo/rest/_connection.py b/openeo/rest/_connection.py index 5b2904b0a..6858b0985 100644 --- a/openeo/rest/_connection.py +++ b/openeo/rest/_connection.py @@ -310,9 +310,9 @@ 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. + # 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): diff --git a/tests/rest/test_job.py b/tests/rest/test_job.py index 334054327..98ab81ffd 100644 --- a/tests/rest/test_job.py +++ b/tests/rest/test_job.py @@ -829,51 +829,41 @@ def test_get_results_download_file_ranged(job_with_chunked_asset_using_head: Bat 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"R4ng3d-D4t4" * 32 # 352 bytes -> one full and one partial 256-byte range - assert len(set(content[i : i + 256] for i in range(0, len(content), 256))) == 2 + content = b"A" * 200 + b"B" * 100 # 300 bytes -> two 200-byte ranges - # HEAD body is ignored, but httpretty validates registered Content-Length against it. + # 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=content.decode("latin-1"), - adding_headers={"Content-Length": f"{len(content)}", "Accept-Ranges": "bytes"}, + 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-255: transient failure. + # First attempt on range 0-199: transient failure. httpretty.Response(status=503, body="Service Unavailable"), - # Retry of range 0-255 succeeds. - httpretty.Response( - status=206, - body=content[0:256].decode("latin-1"), - adding_headers={"Content-Range": f"bytes 0-255/{len(content)}"}, - ), - # Range 256-351 succeeds on the first attempt. - httpretty.Response( - status=206, - body=content[256:].decode("latin-1"), - adding_headers={"Content-Range": f"bytes 256-351/{len(content)}"}, - ), + # 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"}), ], ) - - httpretty.register_uri( - httpretty.GET, - uri=API_URL + "/.well-known/openeo", - body=json.dumps({"api_version": "1.0.0", "endpoints": [{"path": "/credentials/basic", "methods": ["GET"]}]}), - ) + # /.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": [{"path": "/credentials/basic", "methods": ["GET"]}]}), + 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=256) + con.download_url(API_URL + "/dl/ranged.bin", tmp_path / "ranged.bin", range_size=200) assert (tmp_path / "ranged.bin").read_bytes() == content