From db43db684b760bba9ef8469b21057f1ff9989c52 Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Thu, 17 Sep 2026 14:22:56 -0700 Subject: [PATCH 1/3] Recognize 206 responses when probing for HTTP range support A presigned S3 URL is signed for a single method, so the HEAD used to probe for range support is emulated with a one-byte ranged GET. That request is answered with 206, but only 200 was recognized, so opening such a URL fell back to reading the entire resource into memory. Co-Authored-By: Claude Opus 5 (1M context) --- doc/changes/DM-56138.bugfix.rst | 1 + python/lsst/resources/http.py | 6 ++++- tests/test_http.py | 47 +++++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 doc/changes/DM-56138.bugfix.rst diff --git a/doc/changes/DM-56138.bugfix.rst b/doc/changes/DM-56138.bugfix.rst new file mode 100644 index 00000000..731ff286 --- /dev/null +++ b/doc/changes/DM-56138.bugfix.rst @@ -0,0 +1 @@ +Opening a presigned S3 URL over HTTP no longer downloads the entire object into memory. diff --git a/python/lsst/resources/http.py b/python/lsst/resources/http.py index e5b8d65a..c87ef76f 100644 --- a/python/lsst/resources/http.py +++ b/python/lsst/resources/http.py @@ -2052,7 +2052,11 @@ def _openImpl( encoding: str | None = None, ) -> Generator[ResourceHandleProtocol]: resp = self._head() - accepts_range = resp.status_code == requests.codes.ok and resp.headers.get("Accept-Ranges") == "bytes" + # A presigned S3 URL is signed for a single method, so _head() emulates + # HEAD with a one-byte ranged GET, which is answered with 206 rather + # than 200. + range_capable = (requests.codes.ok, requests.codes.partial_content) + accepts_range = resp.status_code in range_capable and resp.headers.get("Accept-Ranges") == "bytes" handle: ResourceHandleProtocol if mode in ("rb", "r") and accepts_range: handle = HttpReadResourceHandle(mode, log, self, timeout=self._config.timeout) diff --git a/tests/test_http.py b/tests/test_http.py index fcbda7bd..b74fb83a 100644 --- a/tests/test_http.py +++ b/tests/test_http.py @@ -14,6 +14,7 @@ import os.path import pickle import random +import re import shutil import socket import stat @@ -155,6 +156,52 @@ def test_get_info(self): self.assertEqual(info.checksums, {"md5": "rL0Y20zC+Fzt72VPzMSk2A==", "sha-256": "def456"}) self.assertEqual(len(responses.calls), 2) + @responses.activate + def test_open_presigned_s3_url_uses_range_requests(self): + """Opening a presigned S3 URL must read byte ranges on demand rather + than downloading the whole object. + """ + _get_dav_and_server_headers.cache_clear() + responses.add(responses.OPTIONS, "http://s3.test/", status=200) + + body = b"0123456789abcdef" + url = "http://s3.test/big.dat?AWSAccessKeyId=key&Signature=sig&Expires=1000" + + def serve_range(request): + # A presigned URL is signed for GET only, so the HEAD used to probe + # for range support is emulated with a one-byte ranged GET. Such a + # request is answered with 206, never 200. + byte_range = request.headers.get("Range") + if byte_range is None: + return (200, {"Accept-Ranges": "bytes"}, body) + # An open-ended range such as "bytes=4-" runs to the end of body. + start, end = re.fullmatch(r"bytes=(\d+)-(\d*)", byte_range).groups() + first = int(start) + if first >= len(body): + return (416, {"Accept-Ranges": "bytes"}, b"") + last = int(end) if end else len(body) - 1 + chunk = body[first : last + 1] + return ( + 206, + { + "Accept-Ranges": "bytes", + "Content-Range": f"bytes {first}-{first + len(chunk) - 1}/{len(body)}", + }, + chunk, + ) + + responses.add_callback(responses.GET, url, callback=serve_range) + + with ResourcePath(url).open("rb") as handle: + self.assertIsInstance(handle, HttpReadResourceHandle) + handle.seek(-4, io.SEEK_END) + self.assertEqual(handle.read(), b"cdef") + + gets = [call.request for call in responses.calls if call.request.method == "GET"] + self.assertTrue(gets) + for request in gets: + self.assertIn("Range", request.headers) + class HttpReadWriteWebdavTestCase(GenericReadWriteTestCase, unittest.TestCase): """Test with a real webDAV server, as opposed to mocking responses.""" From 16d6547f439860c8c4f21497ff728dc68a9f49ba Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Thu, 17 Sep 2026 14:26:10 -0700 Subject: [PATCH 2/3] Reuse the size reported by the HTTP range-support probe The one-byte ranged GET that probes for range support reports the total size of the resource in its Content-Range header, but the handle discarded it and asked the server again the first time a seek relative to the end needed a size. Co-Authored-By: Claude Opus 5 (1M context) --- doc/changes/DM-56138.perf.rst | 2 + .../_resourceHandles/_httpResourceHandle.py | 7 +++- python/lsst/resources/http.py | 27 +++++++++++- tests/test_http.py | 41 +++++++++++++++++++ 4 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 doc/changes/DM-56138.perf.rst diff --git a/doc/changes/DM-56138.perf.rst b/doc/changes/DM-56138.perf.rst new file mode 100644 index 00000000..9acd43a2 --- /dev/null +++ b/doc/changes/DM-56138.perf.rst @@ -0,0 +1,2 @@ +``ResourcePath.open()`` on a plain HTTP URL now passes the resource size already reported by the range-support probe to the returned handle. +Seeking relative to the end of the resource no longer needs an extra request to ask the server for its size. diff --git a/python/lsst/resources/_resourceHandles/_httpResourceHandle.py b/python/lsst/resources/_resourceHandles/_httpResourceHandle.py index f962d790..6d6dcdb9 100644 --- a/python/lsst/resources/_resourceHandles/_httpResourceHandle.py +++ b/python/lsst/resources/_resourceHandles/_httpResourceHandle.py @@ -57,6 +57,10 @@ class HttpReadResourceHandle(BaseResourceHandle[bytes]): Defaults to newline. If a file is opened in binary mode, this argument is not used, as binary files will only split lines on the binary newline representation. + size : `int` or `None`, optional + Total size of the remote resource in bytes, if it is already known. + Saves a request to the server the first time the size is needed, for + example when seeking relative to the end of the resource. """ def __init__( @@ -67,6 +71,7 @@ def __init__( *, timeout: tuple[float, float] | None = None, newline: AnyStr | None = None, + size: int | None = None, ) -> None: super().__init__(mode, log, uri, newline=newline) self._url = uri.geturl() @@ -81,7 +86,7 @@ def __init__( self._closed = CloseStatus.OPEN self._current_position = 0 self._eof = False - self._total_size = -1 # Unknown + self._total_size = -1 if size is None else size # -1 means unknown def close(self) -> None: self._closed = CloseStatus.CLOSED diff --git a/python/lsst/resources/http.py b/python/lsst/resources/http.py index c87ef76f..ef155259 100644 --- a/python/lsst/resources/http.py +++ b/python/lsst/resources/http.py @@ -2059,7 +2059,9 @@ def _openImpl( accepts_range = resp.status_code in range_capable and resp.headers.get("Accept-Ranges") == "bytes" handle: ResourceHandleProtocol if mode in ("rb", "r") and accepts_range: - handle = HttpReadResourceHandle(mode, log, self, timeout=self._config.timeout) + handle = HttpReadResourceHandle( + mode, log, self, timeout=self._config.timeout, size=_total_size_from_partial_content(resp) + ) if mode == "r": # cast because the protocol is compatible, but does not have # BytesIO in the inheritance tree @@ -2075,6 +2077,29 @@ def _copy_extra_attributes(self, original_uri: ResourcePath) -> None: self._extra_headers = original_uri._extra_headers +def _total_size_from_partial_content(resp: requests.Response) -> int | None: + """Return the total size of the resource reported by a 206 response. + + Parameters + ---------- + resp : `requests.Response` + Response to inspect. + + Returns + ------- + size : `int` or `None` + Total size of the resource in bytes, or `None` if the response does + not report one. A 200 response is deliberately ignored because its + 'Content-Length' describes the transferred body, which may be + content-encoded, rather than the resource itself. + """ + if resp.status_code != requests.codes.partial_content: + return None + if (content_range_header := resp.headers.get("Content-Range")) is None: + return None + return parse_content_range_header(content_range_header).total + + def _dump_response(resp: requests.Response) -> None: """Log the contents of a HTTP or webDAV request and its response. diff --git a/tests/test_http.py b/tests/test_http.py index b74fb83a..e60e3bea 100644 --- a/tests/test_http.py +++ b/tests/test_http.py @@ -202,6 +202,47 @@ def serve_range(request): for request in gets: self.assertIn("Range", request.headers) + @responses.activate + def test_open_presigned_s3_url_reuses_probed_size(self): + """The one-byte GET used to probe for range support already reports the + total size, so the handle must not ask the server for it again. + """ + _get_dav_and_server_headers.cache_clear() + responses.add(responses.OPTIONS, "http://s3.test/", status=200) + + body = b"0123456789abcdef" + url = "http://s3.test/big.dat?AWSAccessKeyId=key&Signature=sig&Expires=1000" + + def serve_range(request): + byte_range = request.headers.get("Range") + if byte_range is None: + return (200, {"Accept-Ranges": "bytes"}, body) + start, end = re.fullmatch(r"bytes=(\d+)-(\d*)", byte_range).groups() + first = int(start) + last = int(end) if end else len(body) - 1 + chunk = body[first : last + 1] + return ( + 206, + { + "Accept-Ranges": "bytes", + "Content-Range": f"bytes {first}-{first + len(chunk) - 1}/{len(body)}", + }, + chunk, + ) + + responses.add_callback(responses.GET, url, callback=serve_range) + + with ResourcePath(url).open("rb") as handle: + handle.seek(-4, io.SEEK_END) + self.assertEqual(handle.read(), b"cdef") + + probes = [ + call.request + for call in responses.calls + if call.request.method == "GET" and call.request.headers.get("Range") == "bytes=0-0" + ] + self.assertEqual(len(probes), 1) + class HttpReadWriteWebdavTestCase(GenericReadWriteTestCase, unittest.TestCase): """Test with a real webDAV server, as opposed to mocking responses.""" From 252ff884b488cf7b0a293898c2c4aa440e8ddad2 Mon Sep 17 00:00:00 2001 From: Tim Jenness Date: Tue, 15 Sep 2026 13:54:05 -0700 Subject: [PATCH 3/3] Compare the optional google retry module against None --- python/lsst/resources/gs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/lsst/resources/gs.py b/python/lsst/resources/gs.py index bdcefe55..b35165c0 100644 --- a/python/lsst/resources/gs.py +++ b/python/lsst/resources/gs.py @@ -108,7 +108,7 @@ def is_retryable(exc: Exception) -> bool: return isinstance(exc, _RETRIEVABLE_TYPES) -_RETRY_POLICY = retry.Retry(predicate=is_retryable) if retry else None +_RETRY_POLICY = retry.Retry(predicate=is_retryable) if retry is not None else None _client = None