From 90b6deeb117f5f9a23a4e6914859a9a621beb696 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Tue, 18 Aug 2026 15:54:25 -0400 Subject: [PATCH 01/19] feat: add HTTP caching helpers (Cache-Control, ETag, 304) --- tests/test_utils_caching.py | 97 +++++++++++++++++++++++++++++++++++++ x2s3/utils.py | 80 ++++++++++++++++++++++++++++++ 2 files changed, 177 insertions(+) create mode 100644 tests/test_utils_caching.py diff --git a/tests/test_utils_caching.py b/tests/test_utils_caching.py new file mode 100644 index 0000000..dff3d24 --- /dev/null +++ b/tests/test_utils_caching.py @@ -0,0 +1,97 @@ +from email.utils import parsedate_to_datetime + +from x2s3.utils import ( + CACHE_CONTROL_PUBLIC, + check_not_modified, + format_http_date, + make_file_etag, +) + +ETAG = make_file_etag(1755172800.0, 1234) +LAST_MODIFIED = format_http_date(1755172800.0) +RESPONSE_HEADERS = { + "ETag": ETAG, + "Last-Modified": LAST_MODIFIED, + "Cache-Control": CACHE_CONTROL_PUBLIC, + "Content-Length": "1234", + "Content-Type": "application/octet-stream", +} + + +def test_format_http_date_is_rfc7231(): + # Browsers and shared caches can only use Last-Modified if it parses as an + # HTTP-date; the S3 ISO format does not. + assert LAST_MODIFIED.endswith("GMT") + assert parsedate_to_datetime(LAST_MODIFIED) is not None + + +def test_make_file_etag_is_quoted_and_varies(): + assert ETAG.startswith('"') and ETAG.endswith('"') + assert make_file_etag(1755172800.0, 1234) != make_file_etag(1755172800.0, 1235) + assert make_file_etag(1755172800.0, 1234) != make_file_etag(1755172801.0, 1234) + + +def test_no_validators_means_no_304(): + assert check_not_modified({}, RESPONSE_HEADERS) is None + + +def test_matching_if_none_match_returns_304(): + response = check_not_modified({"if-none-match": ETAG}, RESPONSE_HEADERS) + assert response is not None + assert response.status_code == 304 + + +def test_304_carries_validators_but_no_content_headers(): + response = check_not_modified({"if-none-match": ETAG}, RESPONSE_HEADERS) + assert response.headers["etag"] == ETAG + assert response.headers["last-modified"] == LAST_MODIFIED + assert response.headers["cache-control"] == CACHE_CONTROL_PUBLIC + assert "content-length" not in response.headers + assert "content-type" not in response.headers + + +def test_stale_if_none_match_returns_none(): + assert check_not_modified({"if-none-match": '"nope-1"'}, RESPONSE_HEADERS) is None + + +def test_star_if_none_match_returns_304(): + response = check_not_modified({"if-none-match": "*"}, RESPONSE_HEADERS) + assert response is not None + + +def test_weak_and_list_if_none_match_match(): + response = check_not_modified({"if-none-match": f'"other", W/{ETAG}'}, RESPONSE_HEADERS) + assert response is not None + + +def test_if_none_match_takes_precedence_over_if_modified_since(): + # A non-matching ETag means not-modified is false, even though the + # If-Modified-Since alone would have produced a 304. + headers = {"if-none-match": '"nope-1"', "if-modified-since": LAST_MODIFIED} + assert check_not_modified(headers, RESPONSE_HEADERS) is None + + +def test_if_modified_since_at_or_after_mtime_returns_304(): + assert check_not_modified({"if-modified-since": LAST_MODIFIED}, RESPONSE_HEADERS) is not None + later = format_http_date(1755172800.0 + 60) + assert check_not_modified({"if-modified-since": later}, RESPONSE_HEADERS) is not None + + +def test_if_modified_since_before_mtime_returns_none(): + earlier = format_http_date(1755172800.0 - 60) + assert check_not_modified({"if-modified-since": earlier}, RESPONSE_HEADERS) is None + + +def test_unparseable_if_modified_since_returns_none(): + assert check_not_modified({"if-modified-since": "not a date"}, RESPONSE_HEADERS) is None + + +def test_response_header_lookup_is_case_insensitive(): + # Fileglancer passes a plain dict whose keys came from x2s3, so the helper + # cannot rely on Starlette's case-insensitive Headers mapping. + lowered = {k.lower(): v for k, v in RESPONSE_HEADERS.items()} + assert check_not_modified({"if-none-match": ETAG}, lowered) is not None + + +def test_missing_etag_never_matches(): + assert check_not_modified({"if-none-match": ETAG}, {"Cache-Control": CACHE_CONTROL_PUBLIC}) is None diff --git a/x2s3/utils.py b/x2s3/utils.py index c29a83b..44e723f 100644 --- a/x2s3/utils.py +++ b/x2s3/utils.py @@ -3,6 +3,7 @@ import urllib import xml.etree.ElementTree as ET from datetime import datetime, timezone +from email.utils import formatdate, parsedate_to_datetime from mimetypes import guess_type from html import escape @@ -252,3 +253,82 @@ def guess_content_type(filename): return 'text/plain+yaml' else: return 'application/octet-stream' + + +# Zarr chunks are effectively immutable by path, but datasets are sometimes +# overwritten in place, so one hour is the worst-case staleness we accept in +# exchange for stopping sub-second chunk re-fetch storms. No `immutable`. +CACHE_CONTROL_PUBLIC = "public, max-age=3600" + + +def make_file_etag(mtime: float, size: int) -> str: + """Strong ETag for a file, derived from its mtime and size. + + Same scheme as fileglancer's make_etag, so the two repos agree on the + validator for the same file. + + ponytail: mtime granularity is the ceiling — two writes within the + filesystem's mtime resolution that also keep the same size would share an + ETag. Switch to a content hash if that ever matters. + """ + return f'"{mtime:.6f}-{size}"' + + +def format_http_date(timestamp) -> str: + """Format a POSIX timestamp as an RFC 7231 HTTP-date. + + Distinct from format_timestamp_s3, which produces the ISO form S3 uses in + listing XML bodies. Headers must use this one or caches cannot read them. + """ + return formatdate(timestamp, usegmt=True) + + +def _etag_matches(if_none_match: str, etag: str) -> bool: + """True if any entity-tag in an If-None-Match header matches ours.""" + if not etag: + return False + for candidate in if_none_match.split(','): + candidate = candidate.strip() + if candidate == '*': + return True + if candidate.startswith('W/'): + candidate = candidate[2:] + if candidate == etag: + return True + return False + + +def check_not_modified(request_headers, response_headers): + """Return a 304 response if the request's validators match, else None. + + request_headers is a case-insensitive mapping (Starlette Headers). + response_headers may be a plain dict with canonical capitalization, which + is what Fileglancer receives back from its user worker, so lookups here are + lowercased explicitly. + """ + lowered = {k.lower(): v for k, v in response_headers.items()} + etag = lowered.get('etag') + last_modified = lowered.get('last-modified') + + if_none_match = request_headers.get('if-none-match') + if if_none_match is not None: + # If-None-Match wins outright: when it is present and does not match, + # If-Modified-Since must not be consulted (RFC 9110 13.1.3). + if not _etag_matches(if_none_match, etag): + return None + else: + if_modified_since = request_headers.get('if-modified-since') + if not if_modified_since or not last_modified: + return None + try: + since = parsedate_to_datetime(if_modified_since) + modified = parsedate_to_datetime(last_modified) + except (TypeError, ValueError): + return None + if since is None or modified is None or modified > since: + return None + + headers = {name: lowered[name.lower()] + for name in ('ETag', 'Last-Modified', 'Cache-Control') + if lowered.get(name.lower())} + return Response(status_code=304, headers=headers) From aec0bcbfb28864025af931fd865060c29be86eee Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Tue, 18 Aug 2026 16:03:47 -0400 Subject: [PATCH 02/19] fix: guard naive If-Modified-Since comparison against TypeError parsedate_to_datetime returns a naive datetime for a zoneless date header, and comparing it to our timezone-aware Last-Modified raised an uncaught TypeError, turning a slightly non-conformant conditional GET into a 500. Move the comparison inside the try/except so it degrades to a cache miss instead. Also pin make_file_etag's exact wire format since fileglancer must byte-for-byte match it. Code review round 1. --- tests/test_utils_caching.py | 12 ++++++++++++ x2s3/utils.py | 8 ++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/tests/test_utils_caching.py b/tests/test_utils_caching.py index dff3d24..b6b6fba 100644 --- a/tests/test_utils_caching.py +++ b/tests/test_utils_caching.py @@ -95,3 +95,15 @@ def test_response_header_lookup_is_case_insensitive(): def test_missing_etag_never_matches(): assert check_not_modified({"if-none-match": ETAG}, {"Cache-Control": CACHE_CONTROL_PUBLIC}) is None + + +def test_naive_if_modified_since_does_not_crash(): + # parsedate_to_datetime returns a naive datetime for a zoneless date, and + # comparing it to our timezone-aware one raises TypeError. + assert check_not_modified({"if-modified-since": "Thu, 14 Aug 2026 12:00:00"}, + RESPONSE_HEADERS) is None + + +def test_make_file_etag_wire_format_is_pinned(): + # Fileglancer's make_etag must produce this byte-for-byte for the same file. + assert make_file_etag(1755172800.0, 1234) == '"1755172800.000000-1234"' diff --git a/x2s3/utils.py b/x2s3/utils.py index 44e723f..35fd2dc 100644 --- a/x2s3/utils.py +++ b/x2s3/utils.py @@ -323,9 +323,13 @@ def check_not_modified(request_headers, response_headers): try: since = parsedate_to_datetime(if_modified_since) modified = parsedate_to_datetime(last_modified) + if since is None or modified is None or modified > since: + return None except (TypeError, ValueError): - return None - if since is None or modified is None or modified > since: + # parsedate_to_datetime returns a NAIVE datetime for a date with no + # zone, which clients do send. Comparing it to our aware one raises + # TypeError, so the comparison has to sit inside the guard or a + # slightly-off header becomes a 500. return None headers = {name: lowered[name.lower()] From b01a2b03381988128476ffd14cd50b5671ee08e6 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Tue, 18 Aug 2026 16:07:44 -0400 Subject: [PATCH 03/19] feat: send Cache-Control, ETag, and HTTP-date Last-Modified for files --- tests/test_file.py | 44 ++++++++++++++++++++++++++++++++++++++++++++ x2s3/client_file.py | 8 ++++++-- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/tests/test_file.py b/tests/test_file.py index 5ef3e59..5c2fcfa 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -271,3 +271,47 @@ def test_unbrowseable_head(app): assert response.status_code == 200 response = client.head("/local-files/missing") assert response.status_code == 404 + + +def test_file_get_has_caching_headers(app): + with TestClient(app) as client: + response = client.get("/local-files/README.md") + assert response.status_code == 200 + assert response.headers['cache-control'] == "public, max-age=3600" + assert response.headers['etag'].startswith('"') + + +def test_file_head_has_caching_headers(app): + with TestClient(app) as client: + response = client.head("/local-files/README.md") + assert response.status_code == 200 + assert response.headers['cache-control'] == "public, max-age=3600" + assert response.headers['etag'] == client.get("/local-files/README.md").headers['etag'] + + +def test_file_last_modified_is_http_date(app): + # The S3 ISO format is for listing XML; a header must be an HTTP-date or + # browsers and shared caches cannot use it. + from email.utils import parsedate_to_datetime + with TestClient(app) as client: + response = client.head("/local-files/README.md") + assert response.headers['last-modified'].endswith("GMT") + assert parsedate_to_datetime(response.headers['last-modified']) is not None + + +def test_file_ranged_response_has_caching_headers(app): + with TestClient(app) as client: + response = client.get("/local-files/README.md", headers={"Range": "bytes=0-9"}) + assert response.status_code == 206 + assert response.headers['cache-control'] == "public, max-age=3600" + assert response.headers['etag'].startswith('"') + + +def test_listing_keeps_s3_iso_timestamps(app): + # Only the header format changes; the XML body still speaks S3. + with TestClient(app) as client: + response = client.get("/local-files?list-type=2&prefix=tests/&max-keys=1") + root = parse_xml(response.text) + last_modified = root.find('Contents').find('LastModified').text + assert last_modified.endswith("Z") + assert "GMT" not in last_modified diff --git a/x2s3/client_file.py b/x2s3/client_file.py index 50c3859..b1af536 100644 --- a/x2s3/client_file.py +++ b/x2s3/client_file.py @@ -202,7 +202,9 @@ async def head_object(self, key: str): stats = os.stat(path) file_size = stats.st_size headers["Content-Length"] = str(file_size) - headers["Last-Modified"] = format_timestamp_s3(stats.st_mtime) + headers["Last-Modified"] = format_http_date(stats.st_mtime) + headers["ETag"] = make_file_etag(stats.st_mtime, file_size) + headers["Cache-Control"] = CACHE_CONTROL_PUBLIC return Response(headers=headers) except Exception as e: @@ -233,7 +235,9 @@ async def open_object(self, key: str, range_header: str = None): file_handle = open(path, "rb") stats = os.fstat(file_handle.fileno()) file_size = stats.st_size - headers["Last-Modified"] = format_timestamp_s3(stats.st_mtime) + headers["Last-Modified"] = format_http_date(stats.st_mtime) + headers["ETag"] = make_file_etag(stats.st_mtime, file_size) + headers["Cache-Control"] = CACHE_CONTROL_PUBLIC # Handle range requests if range_header: From b2d720abafba702530ae4eb927f2e6e7b4625921 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Tue, 18 Aug 2026 16:13:04 -0400 Subject: [PATCH 04/19] feat: answer conditional requests with 304 on GET and HEAD --- tests/test_file.py | 68 ++++++++++++++++++++++++++++++++++++++++++++++ x2s3/app.py | 30 ++++++++++++++++---- 2 files changed, 93 insertions(+), 5 deletions(-) diff --git a/tests/test_file.py b/tests/test_file.py index 5c2fcfa..bfc1f6a 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -315,3 +315,71 @@ def test_listing_keeps_s3_iso_timestamps(app): last_modified = root.find('Contents').find('LastModified').text assert last_modified.endswith("Z") assert "GMT" not in last_modified + + +def test_get_returns_304_for_matching_etag(app): + with TestClient(app) as client: + first = client.get("/local-files/README.md") + assert first.status_code == 200 + second = client.get("/local-files/README.md", + headers={"If-None-Match": first.headers['etag']}) + assert second.status_code == 304 + assert second.content == b"" + assert second.headers['cache-control'] == "public, max-age=3600" + + +def test_get_returns_200_for_stale_etag(app): + with TestClient(app) as client: + response = client.get("/local-files/README.md", + headers={"If-None-Match": '"stale-1"'}) + assert response.status_code == 200 + assert response.content + + +def test_get_returns_304_for_if_modified_since(app): + with TestClient(app) as client: + first = client.get("/local-files/README.md") + second = client.get("/local-files/README.md", + headers={"If-Modified-Since": first.headers['last-modified']}) + assert second.status_code == 304 + + +def test_if_none_match_beats_range(app): + # RFC 9110 13.1.3: a matching If-None-Match wins over Range, so this is a + # 304 rather than a 206. + with TestClient(app) as client: + first = client.get("/local-files/README.md") + second = client.get("/local-files/README.md", + headers={"If-None-Match": first.headers['etag'], + "Range": "bytes=0-9"}) + assert second.status_code == 304 + + +def test_head_returns_304_for_matching_etag(app): + with TestClient(app) as client: + first = client.head("/local-files/README.md") + second = client.head("/local-files/README.md", + headers={"If-None-Match": first.headers['etag']}) + assert second.status_code == 304 + + +def test_304_does_not_leak_file_handles(app): + # The handle is opened before the validator check, so the 304 path has to + # close it explicitly. + import gc + import warnings + with TestClient(app) as client: + etag = client.get("/local-files/README.md").headers['etag'] + with warnings.catch_warnings(): + warnings.simplefilter("error", ResourceWarning) + for _ in range(20): + assert client.get("/local-files/README.md", + headers={"If-None-Match": etag}).status_code == 304 + gc.collect() + + +def test_304_not_returned_for_missing_key(app): + with TestClient(app) as client: + response = client.get("/local-files/does-not-exist.txt", + headers={"If-None-Match": "*"}) + assert response.status_code == 404 diff --git a/x2s3/app.py b/x2s3/app.py index e027eef..caa43e5 100644 --- a/x2s3/app.py +++ b/x2s3/app.py @@ -19,6 +19,7 @@ from x2s3.utils import * from x2s3 import client_registry +from x2s3.client import ObjectHandle from x2s3.settings import get_settings, Target # Use uvloop for better async performance @@ -390,11 +391,26 @@ async def target_dispatcher(request: Request, async def get_object_or_denied(key): """GetObject with S3-style 404 masking: on unbrowseable buckets a missing key returns 403 AccessDenied so clients can't probe which - keys exist (real S3 does this when s3:ListBucket is denied).""" - response = await client.get_object(key, request.headers.get("range")) - if response.status_code == 404 and not target_config.browseable: - return get_accessdenied_response() - return response + keys exist (real S3 does this when s3:ListBucket is denied). + + Opens the object first so the validator check can see its ETag, + then either answers 304 or streams. The handle is closed on the + 304 path — nothing else will. + + ponytail: for S3 targets this means an upstream fetch is opened and + abandoned on a 304. Forward IfNoneMatch into client_aioboto if S3 + targets ever become a hot path. + """ + handle = await client.open_object(key, request.headers.get("range")) + if not isinstance(handle, ObjectHandle): + if handle.status_code == 404 and not target_config.browseable: + return get_accessdenied_response() + return handle + not_modified = check_not_modified(request.headers, handle.headers) + if not_modified is not None: + handle.close() + return not_modified + return client.stream_object(handle) if list_type: if not target_path: @@ -467,6 +483,10 @@ async def head_object(request: Request, path: str): if response.status_code == 404 and not target_config.browseable: # Mask missing keys on unbrowseable buckets; HEAD carries no body return Response(status_code=403, media_type="application/xml") + if response.status_code == 200: + not_modified = check_not_modified(request.headers, response.headers) + if not_modified is not None: + return not_modified return response except Exception: logger.opt(exception=sys.exc_info()).info("Error requesting head") From d71556fbbf5a4d1d3f25723dc5cde08322183cc7 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Tue, 18 Aug 2026 16:20:54 -0400 Subject: [PATCH 05/19] fix: replace ineffective 304-leak detector with a close() spy ResourceWarning-based and open-fd-count-based detection both pass even when handle.close() is deleted from the 304 path: CPython's refcounting deallocates the underlying file object (running its own closing finalizer) the instant the local handle variable goes out of scope, so neither detector observes a difference. Spy directly on FileObjectHandle.close() instead, which fails when the call is removed and passes when it is present (verified both ways). --- tests/test_file.py | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/tests/test_file.py b/tests/test_file.py index bfc1f6a..5a50d4c 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -363,19 +363,36 @@ def test_head_returns_304_for_matching_etag(app): assert second.status_code == 304 -def test_304_does_not_leak_file_handles(app): +def test_304_does_not_leak_file_handles(app, monkeypatch): # The handle is opened before the validator check, so the 304 path has to - # close it explicitly. - import gc - import warnings + # close it explicitly. Neither a ResourceWarning trap nor an open-fd count + # can observe this reliably: CPython's refcounting deallocates the + # underlying file object (running its own closing finalizer) the instant + # the local `handle` variable in get_object_or_denied goes out of scope, + # before either detector gets a chance to look. So this spies directly on + # FileObjectHandle.close() to confirm the 304 path actually calls it. + from x2s3.client_file import FileObjectHandle + + calls = [] + original_close = FileObjectHandle.close + + def spy_close(self): + calls.append(self) + original_close(self) + + monkeypatch.setattr(FileObjectHandle, "close", spy_close) + with TestClient(app) as client: + # The initial 200 GET streams its own handle closed when the response + # finishes, which also calls close() — so only count closes seen + # during the 304 loop below, not this warm-up call. etag = client.get("/local-files/README.md").headers['etag'] - with warnings.catch_warnings(): - warnings.simplefilter("error", ResourceWarning) - for _ in range(20): - assert client.get("/local-files/README.md", - headers={"If-None-Match": etag}).status_code == 304 - gc.collect() + before = len(calls) + for _ in range(3): + assert client.get("/local-files/README.md", + headers={"If-None-Match": etag}).status_code == 304 + + assert len(calls) - before == 3 def test_304_not_returned_for_missing_key(app): From 004e18cf2e48892414051c765432663714e75ccc Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Tue, 18 Aug 2026 16:26:49 -0400 Subject: [PATCH 06/19] feat: send Cache-Control for S3-backed targets --- tests/test_awss3.py | 2 ++ x2s3/client_aioboto.py | 2 ++ 2 files changed, 4 insertions(+) diff --git a/tests/test_awss3.py b/tests/test_awss3.py index 888212f..ba217e0 100644 --- a/tests/test_awss3.py +++ b/tests/test_awss3.py @@ -160,6 +160,7 @@ def test_head_object(app): with TestClient(app) as client: response = client.head("/janelia-data-examples/jrc_mus_lung_covid.n5/attributes.json") assert response.status_code == 200 + assert response.headers['cache-control'] == "public, max-age=3600" response = client.head("/janelia-data-examples/jrc_mus_lung_covid.n5/") assert response.status_code == 404 @@ -178,6 +179,7 @@ def test_get_object(app): with TestClient(app) as client: response = client.get("/janelia-data-examples/jrc_mus_lung_covid.n5/attributes.json") assert response.status_code == 200 + assert response.headers['cache-control'] == "public, max-age=3600" json_obj = response.json() assert 'n5' in json_obj diff --git a/x2s3/client_aioboto.py b/x2s3/client_aioboto.py index 116a9b8..a026a6a 100644 --- a/x2s3/client_aioboto.py +++ b/x2s3/client_aioboto.py @@ -170,6 +170,7 @@ async def head_object(self, key: str): "Accept-Ranges": "bytes", "Content-Length": str(s3_res.get("ContentLength")), "Last-Modified": s3_res.get("LastModified").strftime("%a, %d %b %Y %H:%M:%S GMT"), + "Cache-Control": CACHE_CONTROL_PUBLIC, } if self.proxy_etag: @@ -196,6 +197,7 @@ async def open_object(self, key: str, range_header: str = None): headers = { 'Accept-Ranges': "bytes", 'Content-Type': content_type, + 'Cache-Control': CACHE_CONTROL_PUBLIC, } if content_type == 'application/octet-stream': From 302f8be24a9a4c6865fae874a20b240c02360d85 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Tue, 18 Aug 2026 16:31:09 -0400 Subject: [PATCH 07/19] chore: bump version to 1.4.4 --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 27756c0..b0378aa 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "x2s3" -version = "1.4.3" +version = "1.4.4" description = "RESTful web service which makes any storage system X available as an S3-compatible REST API" readme = "README.md" license = { file = "LICENSE" } From 82399c886880ec93d3622d27d8e6cd88dfccb46f Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Tue, 18 Aug 2026 16:48:38 -0400 Subject: [PATCH 08/19] fix: final review fixes for cache-headers branch before 1.4.4 Five fixes from the whole-branch review, applied together since the test suite stays green across all of them: - nginx no longer hides Cache-Control on proxied responses (it was stripping the header this whole branch exists to emit); proxy_ignore_headers Cache-Control is kept so nginx's own proxy_cache_valid is unaffected. - Dispatcher now honors If-Range on file/S3 GETs: a stale If-Range validator makes it drop the Range and re-open for a full 200, per RFC 9110 13.1.5, instead of silently serving partial bytes from a since-overwritten file. - The file client's 416 response no longer advertises Cache-Control/ETag, so a shared cache can't replay a stored 416 for a later plain GET. - Documented why the "-" in make_file_etag is load-bearing: it's what makes the AWS Java SDK v1 skip its (broken) MD5 integrity check. - Restored the close-on-failure guard around stream_object() that the old get_object() convenience method had. --- docker/include/proxy_pass.conf | 4 ++- tests/test_file.py | 51 ++++++++++++++++++++++++++++++++++ tests/test_utils_caching.py | 20 +++++++++++++ x2s3/app.py | 39 ++++++++++++++++++++++---- x2s3/client_file.py | 13 +++++++-- x2s3/utils.py | 31 +++++++++++++++++++++ 6 files changed, 149 insertions(+), 9 deletions(-) diff --git a/docker/include/proxy_pass.conf b/docker/include/proxy_pass.conf index 08bdc79..ce3f08e 100644 --- a/docker/include/proxy_pass.conf +++ b/docker/include/proxy_pass.conf @@ -13,9 +13,11 @@ proxy_ignore_headers X-Accel-Expires; proxy_ignore_headers Cache-Control; proxy_ignore_headers Set-Cookie; +# Cache-Control is intentionally NOT hidden here: x2s3 emits it so browsers +# and shared caches can cache responses. proxy_ignore_headers Cache-Control +# above still keeps nginx's own proxy_cache_valid in force regardless. proxy_hide_header Expires; proxy_hide_header X-Accel-Expires; -proxy_hide_header Cache-Control; proxy_hide_header Pragma; # Replace CORS headers diff --git a/tests/test_file.py b/tests/test_file.py index 5a50d4c..0fd4435 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -400,3 +400,54 @@ def test_304_not_returned_for_missing_key(app): response = client.get("/local-files/does-not-exist.txt", headers={"If-None-Match": "*"}) assert response.status_code == 404 + + +def test_416_response_is_not_cacheable(app): + # An explicitly cacheable 416 could be replayed by a shared cache for a + # later plain GET (caches key on URI+method, not Range), making the + # object look permanently broken. + with TestClient(app) as client: + response = client.get("/local-files/README.md", + headers={"Range": "bytes=99999999-100000000"}) + assert response.status_code == 416 + assert 'cache-control' not in response.headers + assert 'etag' not in response.headers + + +def test_if_range_matching_etag_returns_ranged_body(app): + with TestClient(app) as client: + full = client.get("/local-files/README.md") + response = client.get("/local-files/README.md", + headers={"Range": "bytes=0-9", + "If-Range": full.headers['etag']}) + assert response.status_code == 206 + assert response.content == full.content[:10] + + +def test_if_range_stale_etag_returns_full_body(app): + with TestClient(app) as client: + full = client.get("/local-files/README.md") + response = client.get("/local-files/README.md", + headers={"Range": "bytes=0-9", + "If-Range": '"stale-etag"'}) + assert response.status_code == 200 + assert response.content == full.content + + +def test_if_range_without_range_is_ignored(app): + with TestClient(app) as client: + full = client.get("/local-files/README.md") + response = client.get("/local-files/README.md", + headers={"If-Range": '"stale-etag"'}) + assert response.status_code == 200 + assert response.content == full.content + + +def test_if_range_matching_last_modified_returns_ranged_body(app): + with TestClient(app) as client: + full = client.get("/local-files/README.md") + response = client.get("/local-files/README.md", + headers={"Range": "bytes=0-9", + "If-Range": full.headers['last-modified']}) + assert response.status_code == 206 + assert response.content == full.content[:10] diff --git a/tests/test_utils_caching.py b/tests/test_utils_caching.py index b6b6fba..57fb711 100644 --- a/tests/test_utils_caching.py +++ b/tests/test_utils_caching.py @@ -4,6 +4,7 @@ CACHE_CONTROL_PUBLIC, check_not_modified, format_http_date, + if_range_matches, make_file_etag, ) @@ -107,3 +108,22 @@ def test_naive_if_modified_since_does_not_crash(): def test_make_file_etag_wire_format_is_pinned(): # Fileglancer's make_etag must produce this byte-for-byte for the same file. assert make_file_etag(1755172800.0, 1234) == '"1755172800.000000-1234"' + + +def test_if_range_matches_etag(): + assert if_range_matches(ETAG, RESPONSE_HEADERS) is True + + +def test_if_range_matches_last_modified(): + assert if_range_matches(LAST_MODIFIED, RESPONSE_HEADERS) is True + + +def test_if_range_stale_validator_does_not_match(): + assert if_range_matches('"stale-etag"', RESPONSE_HEADERS) is False + + +def test_if_range_weak_etag_never_matches(): + # RFC 9110 13.1.5: a weak validator is never valid in If-Range, even if + # its underlying tag matches -- so this is a plain string comparison, + # not the weak-stripping If-None-Match does. + assert if_range_matches(f"W/{ETAG}", RESPONSE_HEADERS) is False diff --git a/x2s3/app.py b/x2s3/app.py index caa43e5..8c2949c 100644 --- a/x2s3/app.py +++ b/x2s3/app.py @@ -388,6 +388,17 @@ async def target_dispatcher(request: Request, return get_accessdenied_response() return get_read_access_acl() + def _handle_or_denied(handle): + """Apply the same 404-masking rule to an open_object() result. + Returns a Response to short-circuit with, or None if handle is a + real ObjectHandle that the caller should keep going with. + """ + if not isinstance(handle, ObjectHandle): + if handle.status_code == 404 and not target_config.browseable: + return get_accessdenied_response() + return handle + return None + async def get_object_or_denied(key): """GetObject with S3-style 404 masking: on unbrowseable buckets a missing key returns 403 AccessDenied so clients can't probe which @@ -402,15 +413,33 @@ async def get_object_or_denied(key): targets ever become a hot path. """ handle = await client.open_object(key, request.headers.get("range")) - if not isinstance(handle, ObjectHandle): - if handle.status_code == 404 and not target_config.browseable: - return get_accessdenied_response() - return handle + denied = _handle_or_denied(handle) + if denied is not None: + return denied + + if_range = request.headers.get("if-range") + if if_range is not None and handle.status_code == 206 \ + and not if_range_matches(if_range, handle.headers): + # RFC 9110 13.1.5: a stale If-Range validator means the Range + # must be ignored and the full representation served instead — + # otherwise a client resuming a partial download against a + # file that's since been overwritten would silently stitch + # bytes from two versions into one corrupt chunk. + handle.close() + handle = await client.open_object(key, None) + denied = _handle_or_denied(handle) + if denied is not None: + return denied + not_modified = check_not_modified(request.headers, handle.headers) if not_modified is not None: handle.close() return not_modified - return client.stream_object(handle) + try: + return client.stream_object(handle) + except Exception: + handle.close() + raise if list_type: if not target_path: diff --git a/x2s3/client_file.py b/x2s3/client_file.py index b1af536..313994e 100644 --- a/x2s3/client_file.py +++ b/x2s3/client_file.py @@ -243,12 +243,19 @@ async def open_object(self, key: str, range_header: str = None): if range_header: range_result = parse_range_header(range_header, file_size) if range_result is None: - # Invalid range, return 416 Range Not Satisfiable + # Invalid range, return 416 Range Not Satisfiable. + # Cache-Control/ETag are dropped here (unlike the 200/206 + # branches below): a shared cache keys on URI+method, not + # Range, so an explicitly cacheable 416 could be served + # back for a later plain GET and make the object look + # broken for an hour. file_handle.close() - headers["Content-Range"] = f"bytes */{file_size}" + error_headers = {k: v for k, v in headers.items() + if k not in ("Cache-Control", "ETag")} + error_headers["Content-Range"] = f"bytes */{file_size}" return Response( status_code=416, - headers=headers + headers=error_headers ) start, end = range_result diff --git a/x2s3/utils.py b/x2s3/utils.py index 35fd2dc..8dc10c0 100644 --- a/x2s3/utils.py +++ b/x2s3/utils.py @@ -270,6 +270,16 @@ def make_file_etag(mtime: float, size: int) -> str: ponytail: mtime granularity is the ceiling — two writes within the filesystem's mtime resolution that also keep the same size would share an ETag. Switch to a content hash if that ever matters. + + The "-" separator is load-bearing, not cosmetic: the AWS Java SDK v1 + skips its MD5 integrity check whenever `eTag.contains("-")`, on the + assumption that a hyphen means a multipart-upload ETag (which isn't an + MD5 of the body). That skip is the only reason handing out an ETag here + doesn't reintroduce the "Unable to verify integrity of data download" + failure that tests/java/.../S3v1IntegrityTest.java exists to reproduce, + and that `proxy_etag=False` defaults exist to avoid. Changing the + separator to `_` or `:` would make the SDK run the MD5 check again and + fail it, breaking Fiji/N5 Viewer. """ return f'"{mtime:.6f}-{size}"' @@ -298,6 +308,27 @@ def _etag_matches(if_none_match: str, etag: str) -> bool: return False +def if_range_matches(if_range: str, response_headers) -> bool: + """True if an If-Range validator exactly matches ETag or Last-Modified. + + response_headers may be a plain dict with canonical capitalization (see + check_not_modified above), so lookups here are lowercased explicitly. + + Unlike If-None-Match, If-Range carries exactly one validator, never a + comma-separated list, and a weak ETag (W/"...") is never a valid match + (RFC 9110 13.1.5). So this is just two exact string comparisons: against + ETag, then against Last-Modified. + """ + lowered = {k.lower(): v for k, v in response_headers.items()} + etag = lowered.get('etag') + last_modified = lowered.get('last-modified') + if etag is not None and if_range == etag: + return True + if last_modified is not None and if_range == last_modified: + return True + return False + + def check_not_modified(request_headers, response_headers): """Return a 304 response if the request's validators match, else None. From 2a447e838d06fda337067c9a28842f8bf27d013e Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 19 Aug 2026 10:27:15 -0400 Subject: [PATCH 09/19] fix: evaluate conditional headers before answering 416 A 416 from an unsatisfiable Range short-circuited all conditional handling: RFC 9110 13.2.2 evaluates If-None-Match/If-Modified-Since (-> 304) and a stale If-Range (-> ignore the Range and serve the full body) before Range, so a client resuming a download of a file that had shrunk got a permanent 416 instead of the full new representation. Co-Authored-By: Claude Fable 5 --- tests/test_file.py | 40 ++++++++++++++++++++++++++++++++++++++++ x2s3/app.py | 27 +++++++++++++++++++++++++-- 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/tests/test_file.py b/tests/test_file.py index 0fd4435..3ce3cb3 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -414,6 +414,46 @@ def test_416_response_is_not_cacheable(app): assert 'etag' not in response.headers +def test_if_none_match_beats_unsatisfiable_range(app): + # RFC 9110 13.2.2 evaluates If-None-Match before Range, so a matching + # validator yields 304 even when the Range is unsatisfiable. + with TestClient(app) as client: + first = client.get("/local-files/README.md") + second = client.get("/local-files/README.md", + headers={"If-None-Match": first.headers['etag'], + "Range": "bytes=99999999-100000000"}) + assert second.status_code == 304 + + +def test_if_range_stale_with_unsatisfiable_range_returns_full_body(app): + # RFC 9110 13.1.5: a stale If-Range means the Range is ignored entirely, + # including one that would otherwise be unsatisfiable. + with TestClient(app) as client: + full = client.get("/local-files/README.md") + response = client.get("/local-files/README.md", + headers={"Range": "bytes=99999999-100000000", + "If-Range": '"stale-etag"'}) + assert response.status_code == 200 + assert response.content == full.content + + +def test_unsatisfiable_range_with_fresh_if_range_still_416(app): + with TestClient(app) as client: + full = client.get("/local-files/README.md") + response = client.get("/local-files/README.md", + headers={"Range": "bytes=99999999-100000000", + "If-Range": full.headers['etag']}) + assert response.status_code == 416 + + +def test_unsatisfiable_range_with_stale_if_none_match_still_416(app): + with TestClient(app) as client: + response = client.get("/local-files/README.md", + headers={"Range": "bytes=99999999-100000000", + "If-None-Match": '"stale-1"'}) + assert response.status_code == 416 + + def test_if_range_matching_etag_returns_ranged_body(app): with TestClient(app) as client: full = client.get("/local-files/README.md") diff --git a/x2s3/app.py b/x2s3/app.py index 8c2949c..bf9af4e 100644 --- a/x2s3/app.py +++ b/x2s3/app.py @@ -414,11 +414,34 @@ async def get_object_or_denied(key): """ handle = await client.open_object(key, request.headers.get("range")) denied = _handle_or_denied(handle) - if denied is not None: + if denied is not None and denied.status_code != 416: return denied if_range = request.headers.get("if-range") - if if_range is not None and handle.status_code == 206 \ + if denied is not None: + # 416 from an unsatisfiable Range. RFC 9110 13.2.2 evaluates + # If-None-Match/If-Modified-Since (-> 304) and a stale + # If-Range (-> ignore the Range, serve the full body) before + # Range, so the 416 can't short-circuit them. It also carries + # no validators (see the client 416 branches), so reopen + # without the Range to get some. + if if_range is None \ + and "if-none-match" not in request.headers \ + and "if-modified-since" not in request.headers: + return denied + handle = await client.open_object(key, None) + full_denied = _handle_or_denied(handle) + if full_denied is not None: + return full_denied + if (if_range is None or if_range_matches(if_range, handle.headers)) \ + and check_not_modified(request.headers, handle.headers) is None: + # Validators say the client's copy is stale and the Range + # still applies, so it is still unsatisfiable. + handle.close() + return denied + # Fall through: check_not_modified below answers the 304, or + # a stale If-Range means this full handle streams as a 200. + elif if_range is not None and handle.status_code == 206 \ and not if_range_matches(if_range, handle.headers): # RFC 9110 13.1.5: a stale If-Range validator means the Range # must be ignored and the full representation served instead — From 0ff3c6ac0f410235103c1c67731e167c283fbd12 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 19 Aug 2026 10:30:16 -0400 Subject: [PATCH 10/19] fix: locale-safe Last-Modified in S3 head_object strftime('%a, %d %b %Y ...') expands day/month names using the process locale, so under a non-C locale the header is not a valid HTTP-date and caches cannot revalidate against it. Use format_http_date (added on this branch, always English/GMT), matching what open_object already proxies. Co-Authored-By: Claude Fable 5 --- tests/test_boto.py | 31 +++++++++++++++++++++++++++++++ x2s3/client_aioboto.py | 3 ++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/tests/test_boto.py b/tests/test_boto.py index be3f785..0d5b792 100644 --- a/tests/test_boto.py +++ b/tests/test_boto.py @@ -165,3 +165,34 @@ def test_get_object_precedence(app, s3_client): assert response['ResponseMetadata']['HTTPStatusCode'] == 200 json_obj = response['Body'].read().decode('utf-8') assert 'n5' in json_obj + + +def test_head_object_last_modified_is_locale_independent(): + # Pure unit test, no server needed. botocore hands head_object a real + # datetime, and formatting it with strftime('%a, %d %b %Y ...') expands + # %a/%b using the process locale — under e.g. LC_TIME=de_DE that yields + # '.., 18 Dez ...', which no cache or client can parse, silently killing + # 304 revalidation for S3 targets. No non-English locale is installed in + # CI, so simulate one with a datetime whose strftime is locale-poisoned. + import asyncio + from datetime import datetime, timezone + from email.utils import parsedate_to_datetime + + from x2s3.client_aioboto import AiobotoProxyClient + + class GermanLocaleDatetime(datetime): + def strftime(self, fmt): + return super().strftime(fmt).replace('Dec', 'Dez') + + last_modified = GermanLocaleDatetime(2026, 12, 18, 12, 0, 0, + tzinfo=timezone.utc) + + class StubS3: + async def head_object(self, **kwargs): + return {"ContentLength": 1234, "LastModified": last_modified} + + client = AiobotoProxyClient({'target_name': 'test'}, bucket='test-bucket') + client.client = StubS3() + + response = asyncio.run(client.head_object('some/key.json')) + assert parsedate_to_datetime(response.headers['last-modified']) == last_modified diff --git a/x2s3/client_aioboto.py b/x2s3/client_aioboto.py index a026a6a..682fcd7 100644 --- a/x2s3/client_aioboto.py +++ b/x2s3/client_aioboto.py @@ -169,7 +169,8 @@ async def head_object(self, key: str): headers = { "Accept-Ranges": "bytes", "Content-Length": str(s3_res.get("ContentLength")), - "Last-Modified": s3_res.get("LastModified").strftime("%a, %d %b %Y %H:%M:%S GMT"), + # format_http_date, not strftime: %a/%b are locale-dependent + "Last-Modified": format_http_date(s3_res.get("LastModified").timestamp()), "Cache-Control": CACHE_CONTROL_PUBLIC, } From aff64b8d6edfa88061ec563220beef95dd202f32 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 19 Aug 2026 10:31:25 -0400 Subject: [PATCH 11/19] fix: If-None-Match '*' matches representations without an ETag The empty-etag guard ran before the '*' check, so 'If-None-Match: *' never produced a 304 on targets that don't proxy ETags (the proxy_etag=False default for S3 targets). RFC 9110 13.1.2: '*' matches any existing representation. Co-Authored-By: Claude Fable 5 --- tests/test_utils_caching.py | 9 +++++++++ x2s3/utils.py | 6 ++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/test_utils_caching.py b/tests/test_utils_caching.py index 57fb711..f387352 100644 --- a/tests/test_utils_caching.py +++ b/tests/test_utils_caching.py @@ -127,3 +127,12 @@ def test_if_range_weak_etag_never_matches(): # its underlying tag matches -- so this is a plain string comparison, # not the weak-stripping If-None-Match does. assert if_range_matches(f"W/{ETAG}", RESPONSE_HEADERS) is False + + +def test_star_if_none_match_matches_even_without_etag(): + # RFC 9110 13.1.2: '*' matches any existing representation, so it must + # yield a 304 even when the response carries no ETag — the default for + # S3 targets, which ship with proxy_etag=False. + response = check_not_modified({"if-none-match": "*"}, + {"Last-Modified": LAST_MODIFIED}) + assert response is not None and response.status_code == 304 diff --git a/x2s3/utils.py b/x2s3/utils.py index 8dc10c0..966d238 100644 --- a/x2s3/utils.py +++ b/x2s3/utils.py @@ -295,12 +295,14 @@ def format_http_date(timestamp) -> str: def _etag_matches(if_none_match: str, etag: str) -> bool: """True if any entity-tag in an If-None-Match header matches ours.""" - if not etag: - return False for candidate in if_none_match.split(','): candidate = candidate.strip() if candidate == '*': + # '*' matches any existing representation (RFC 9110 13.1.2), + # even one carrying no ETag — the guard below must not run first. return True + if not etag: + continue if candidate.startswith('W/'): candidate = candidate[2:] if candidate == etag: From 9639626f15a57f96f0c3194eeefe4aa749033435 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 19 Aug 2026 16:12:30 -0400 Subject: [PATCH 12/19] fix: let client validators reach x2s3 on ranged requests Configuring proxy_cache makes nginx replace the client's If-None-Match and If-Modified-Since with its own revalidation values. Like the Range stripping already handled here, that is decided at config time, so it applied to the ranged requests proxy_cache_bypass sends straight through -- the client's validators were dropped and x2s3 never saw them, making the Range + If-None-Match -> 304 support added on this branch unreachable behind the production nginx. Restore them for ranged requests only. On an unranged request nginx owns these headers: with proxy_cache_revalidate on, forwarding the client's validator would let it answer nginx's own revalidation, so a 304 meant for the client would mark a stale cache entry fresh. Verified against real nginx: a ranged conditional GET returns 206 before this change and 304 after. Co-Authored-By: Claude Fable 5 --- docker/include/proxy_cache.conf | 9 ++ docker/include/proxy_cache_maps.conf | 30 +++++ docker/nginx.conf | 4 + tests/test_nginx_cache_headers.py | 165 +++++++++++++++++++++++++++ 4 files changed, 208 insertions(+) create mode 100644 docker/include/proxy_cache_maps.conf create mode 100644 tests/test_nginx_cache_headers.py diff --git a/docker/include/proxy_cache.conf b/docker/include/proxy_cache.conf index 6abf8ed..a93ece8 100644 --- a/docker/include/proxy_cache.conf +++ b/docker/include/proxy_cache.conf @@ -35,6 +35,15 @@ proxy_no_cache $http_range; proxy_set_header Range $http_range; proxy_set_header If-Range $http_if_range; +# Same story for the other conditional headers: proxy_cache makes nginx replace +# them with its own revalidation values, so on a cache-bypassed ranged request +# the client's validators never reach x2s3 and it cannot answer the 304 that +# RFC 9110 13.2.2 requires. These variables restore them for ranged requests +# only and leave nginx's own revalidation alone otherwise -- see +# proxy_cache_maps.conf, which defines them and explains why. +proxy_set_header If-None-Match $proxy_if_none_match; +proxy_set_header If-Modified-Since $proxy_if_modified_since; + # Set back a nice HTTP Header to indicate what the cache status was add_header X-Proxy-Cache $upstream_cache_status; diff --git a/docker/include/proxy_cache_maps.conf b/docker/include/proxy_cache_maps.conf new file mode 100644 index 0000000..5cd9a05 --- /dev/null +++ b/docker/include/proxy_cache_maps.conf @@ -0,0 +1,30 @@ +# Conditional-request headers, split out because map is http-context only. +# Included from nginx.conf; consumed by the proxy_set_header lines in +# proxy_cache.conf. +# +# Configuring proxy_cache makes nginx replace the client's If-None-Match and +# If-Modified-Since with its own revalidation values ($upstream_cache_etag and +# $upstream_cache_last_modified). Like the Range stripping described in +# proxy_cache.conf, that is decided at config time, so it applies even to the +# ranged requests proxy_cache_bypass sends straight through -- there the +# client's validators are dropped and x2s3 never sees them, so it cannot answer +# the 304 that RFC 9110 13.2.2 requires when Range and If-None-Match arrive +# together. +# +# Restoring them for ranged requests only is deliberate. On an unranged request +# nginx owns these headers: with proxy_cache_revalidate on, a stale entry is +# revalidated using the ETag nginx itself cached. Forwarding the client's +# validator there would let it answer nginx's question, so a 304 meant for the +# client would mark a stale entry fresh and keep serving its old bytes for +# another proxy_cache_valid period. +# +# The '' branches reproduce nginx's built-in defaults exactly. +map $http_range $proxy_if_none_match { + '' $upstream_cache_etag; + default $http_if_none_match; +} + +map $http_range $proxy_if_modified_since { + '' $upstream_cache_last_modified; + default $http_if_modified_since; +} diff --git a/docker/nginx.conf b/docker/nginx.conf index 63342c8..186e656 100644 --- a/docker/nginx.conf +++ b/docker/nginx.conf @@ -16,6 +16,10 @@ http { } proxy_cache_bypass $cache_bypass; + # map is http-context only, so the conditional-header maps consumed by + # proxy_cache.conf have to be included out here + include /etc/nginx/conf/proxy_cache_maps.conf; + # Increase in-memory buffers (this assumes we have a lot of RAM) proxy_buffer_size 64k; proxy_buffers 32 128k; diff --git a/tests/test_nginx_cache_headers.py b/tests/test_nginx_cache_headers.py new file mode 100644 index 0000000..692d5f4 --- /dev/null +++ b/tests/test_nginx_cache_headers.py @@ -0,0 +1,165 @@ +"""Tests for the nginx conditional-header handling in docker/include/. + +nginx replaces the client's If-None-Match/If-Modified-Since with its own +cache-revalidation values whenever proxy_cache is configured, which is decided +at config time. These tests run real nginx in front of an echo upstream and +assert on the headers that actually arrive, since that substitution is +invisible from either end alone. + +Skipped when no nginx binary is installed. +""" + +import json +import shutil +import socket +import subprocess +import threading +import time +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer +from pathlib import Path +from urllib.request import Request, urlopen + +import pytest + +DOCKER_INCLUDE = Path(__file__).resolve().parent.parent / "docker" / "include" +UPSTREAM_ETAG = '"upstream-etag"' +CLIENT_ETAG = '"client-etag"' + +pytestmark = pytest.mark.skipif(shutil.which("nginx") is None, + reason="nginx binary not installed") + + +def _free_port(): + with socket.socket() as s: + s.bind(("127.0.0.1", 0)) + return s.getsockname()[1] + + +class _EchoHandler(BaseHTTPRequestHandler): + """Reflects the request headers it received back as a JSON body.""" + + def do_GET(self): + body = json.dumps(dict(self.headers)).encode() + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(body))) + self.send_header("ETag", UPSTREAM_ETAG) + self.end_headers() + self.wfile.write(body) + + def log_message(self, *args): + pass + + +@pytest.fixture(scope="module") +def upstream(): + server = ThreadingHTTPServer(("127.0.0.1", 0), _EchoHandler) + threading.Thread(target=server.serve_forever, daemon=True).start() + yield server.server_address[1] + server.shutdown() + + +@pytest.fixture(scope="module") +def nginx(upstream, tmp_path_factory): + root = tmp_path_factory.mktemp("nginx") + port = _free_port() + # proxy_cache_valid is first-match-wins, so this 1s entry must precede the + # 15m one inside proxy_cache.conf for the revalidation test to be quick. + (root / "nginx.conf").write_text(f""" +worker_processes 1; +pid {root}/nginx.pid; +error_log {root}/error.log warn; +events {{ worker_connections 64; }} +http {{ + access_log off; + client_body_temp_path {root}/client_temp; + proxy_temp_path {root}/proxy_temp; + fastcgi_temp_path {root}/fastcgi_temp; + uwsgi_temp_path {root}/uwsgi_temp; + scgi_temp_path {root}/scgi_temp; + proxy_cache_path {root}/cache keys_zone=mycache:10m max_size=32m levels=1:2 inactive=1h; + + include {DOCKER_INCLUDE}/proxy_cache_maps.conf; + + server {{ + listen 127.0.0.1:{port}; + location / {{ + proxy_cache_valid 200 1s; + include {DOCKER_INCLUDE}/proxy_cache.conf; + proxy_pass http://127.0.0.1:{upstream}; + }} + }} +}} +""") + check = subprocess.run(["nginx", "-t", "-c", str(root / "nginx.conf")], + capture_output=True, text=True) + assert check.returncode == 0, check.stderr + subprocess.run(["nginx", "-c", str(root / "nginx.conf")], + capture_output=True, text=True, check=True) + for _ in range(50): + try: + with socket.create_connection(("127.0.0.1", port), timeout=0.1): + break + except OSError: + time.sleep(0.1) + yield port + subprocess.run(["nginx", "-c", str(root / "nginx.conf"), "-s", "quit"], + capture_output=True) + + +def request_through_nginx(port, path, **headers): + """GET through nginx, returning (cache status, headers the upstream saw).""" + request = Request(f"http://127.0.0.1:{port}{path}", headers=headers) + with urlopen(request) as response: + seen = {k.lower(): v for k, v in json.loads(response.read()).items()} + return response.headers.get("X-Proxy-Cache"), seen + + +def upstream_headers(port, path, **headers): + """GET through nginx and return the headers the upstream actually saw.""" + return request_through_nginx(port, path, **headers)[1] + + +def test_ranged_request_forwards_client_if_none_match(nginx): + # Range requests bypass the cache entirely, so nginx is a pass-through and + # the client's validator has to survive the hop or x2s3 can never answer + # the 304 that RFC 9110 13.2.2 requires. + seen = upstream_headers(nginx, "/ranged-inm", Range="bytes=0-9", + **{"If-None-Match": CLIENT_ETAG}) + assert seen.get("if-none-match") == CLIENT_ETAG + assert seen.get("range") == "bytes=0-9" + + +def test_ranged_request_forwards_client_if_modified_since(nginx): + since = "Tue, 18 Aug 2026 12:00:00 GMT" + seen = upstream_headers(nginx, "/ranged-ims", Range="bytes=0-9", + **{"If-Modified-Since": since}) + assert seen.get("if-modified-since") == since + + +def test_cached_request_does_not_leak_client_if_none_match(nginx): + # Unranged requests take part in nginx's cache, where these headers belong + # to nginx's own revalidation. Forwarding the client's validator here would + # let it answer nginx's question and revive a stale cache entry. + seen = upstream_headers(nginx, "/cached-inm", **{"If-None-Match": CLIENT_ETAG}) + assert "if-none-match" not in seen + + +def test_cache_revalidation_uses_nginx_own_validator(nginx): + # The regression guard for the map's '' branch: once the entry goes stale, + # nginx must revalidate with the ETag it cached, not the client's, or a 304 + # meant for the client would mark a stale entry fresh. + # + # Expiry is polled rather than slept through: nginx derives validity from + # the upstream Date header, which is whole-second, so a 1s entry expires + # anywhere in roughly 1-3s. + request_through_nginx(nginx, "/reval") + deadline = time.time() + 15 + while time.time() < deadline: + status, seen = request_through_nginx(nginx, "/reval", + **{"If-None-Match": CLIENT_ETAG}) + if status != "HIT": + assert seen.get("if-none-match") == UPSTREAM_ETAG + return + time.sleep(0.25) + pytest.fail("cache entry never expired, so revalidation was never exercised") From 7ffefa1653099a7d973fd847bf590c02669a3d8d Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 19 Aug 2026 21:59:15 -0400 Subject: [PATCH 13/19] fix: keep the origin's Cache-Control on S3-backed responses x2s3 stamped 'public, max-age=3600' on every S3-backed response, overriding whatever the origin had set. Combined with nginx no longer hiding Cache-Control, an object stored no-store or private was re-advertised to browsers and shared caches as publicly cacheable for an hour, so an in-place overwrite kept serving stale bytes. The default now applies only to objects that carry no policy of their own. Co-Authored-By: Claude Fable 5 --- tests/test_boto.py | 95 +++++++++++++++++++++++++++++++++--------- x2s3/client_aioboto.py | 11 ++++- 2 files changed, 86 insertions(+), 20 deletions(-) diff --git a/tests/test_boto.py b/tests/test_boto.py index 0d5b792..1772f01 100644 --- a/tests/test_boto.py +++ b/tests/test_boto.py @@ -1,12 +1,17 @@ +import asyncio import time import multiprocessing +from datetime import datetime, timezone +from email.utils import parsedate_to_datetime import boto3 import pytest from pydantic import HttpUrl from x2s3.app import create_app +from x2s3.client_aioboto import AiobotoProxyClient from x2s3.settings import Target, Settings +from x2s3.utils import CACHE_CONTROL_PUBLIC # Set the start method to spawn to avoid pickling issues multiprocessing.set_start_method('spawn', force=True) @@ -167,32 +172,84 @@ def test_get_object_precedence(app, s3_client): assert 'n5' in json_obj -def test_head_object_last_modified_is_locale_independent(): - # Pure unit test, no server needed. botocore hands head_object a real - # datetime, and formatting it with strftime('%a, %d %b %Y ...') expands - # %a/%b using the process locale — under e.g. LC_TIME=de_DE that yields - # '.., 18 Dez ...', which no cache or client can parse, silently killing - # 304 revalidation for S3 targets. No non-English locale is installed in - # CI, so simulate one with a datetime whose strftime is locale-poisoned. - import asyncio - from datetime import datetime, timezone - from email.utils import parsedate_to_datetime - - from x2s3.client_aioboto import AiobotoProxyClient +# The tests below drive AiobotoProxyClient against a stub and need no network. + + +class _StubS3: + """Minimal stand-in for the aiobotocore client, recording its calls.""" + + def __init__(self, get_headers=None, head_response=None): + self.get_headers = get_headers or {} + self.head_response = head_response or {} + self.calls = [] + + async def get_object(self, **kwargs): + self.calls.append(kwargs) + return {"ResponseMetadata": {"HTTPHeaders": self.get_headers}, + "Body": None} + + async def head_object(self, **kwargs): + self.calls.append(kwargs) + return self.head_response + + +def call_with_stub(stub, method, *args, **options): + """Await one AiobotoProxyClient method against a stub S3 client. + + The client is built inside the coroutine on purpose: asyncio.Lock() binds + to the running loop on Python 3.9 and asyncio.run() leaves no current loop + behind, so building it outside would break whichever test ran second. + """ + async def run(): + client = AiobotoProxyClient({'target_name': 'test'}, + bucket='test-bucket', **options) + client.client = stub + return await getattr(client, method)(*args) + return asyncio.run(run()) + + +def test_head_object_last_modified_is_locale_independent(): + # botocore hands head_object a real datetime, and formatting it with + # strftime('%a, %d %b %Y ...') expands %a/%b using the process locale -- + # under e.g. LC_TIME=de_DE that yields '.., 18 Dez ...', which no cache or + # client can parse, silently killing 304 revalidation for S3 targets. No + # non-English locale is installed in CI, so simulate one with a datetime + # whose strftime is locale-poisoned. class GermanLocaleDatetime(datetime): def strftime(self, fmt): return super().strftime(fmt).replace('Dec', 'Dez') last_modified = GermanLocaleDatetime(2026, 12, 18, 12, 0, 0, tzinfo=timezone.utc) + stub = _StubS3(head_response={"ContentLength": 1234, + "LastModified": last_modified}) - class StubS3: - async def head_object(self, **kwargs): - return {"ContentLength": 1234, "LastModified": last_modified} + response = call_with_stub(stub, 'head_object', 'some/key.json') + assert parsedate_to_datetime(response.headers['last-modified']) == last_modified - client = AiobotoProxyClient({'target_name': 'test'}, bucket='test-bucket') - client.client = StubS3() - response = asyncio.run(client.head_object('some/key.json')) - assert parsedate_to_datetime(response.headers['last-modified']) == last_modified +def test_get_object_keeps_upstream_cache_control(): + # An object the origin marked uncacheable must not be re-advertised as + # publicly cacheable for an hour: the browser and the shared nginx cache in + # front of x2s3 would both retain content the origin said not to store. + stub = _StubS3(get_headers={"content-length": "10", + "cache-control": "no-store"}) + handle = call_with_stub(stub, 'open_object', 'some/key.json') + assert handle.headers["Cache-Control"] == "no-store" + + +def test_get_object_defaults_cache_control_when_upstream_has_none(): + stub = _StubS3(get_headers={"content-length": "10"}) + handle = call_with_stub(stub, 'open_object', 'some/key.json') + assert handle.headers["Cache-Control"] == CACHE_CONTROL_PUBLIC + + +def test_head_object_keeps_upstream_cache_control(): + stub = _StubS3(head_response={ + "ContentLength": 10, + "LastModified": datetime(2026, 8, 18, 12, 0, 0, tzinfo=timezone.utc), + "CacheControl": "private, max-age=60", + }) + response = call_with_stub(stub, 'head_object', 'some/key.json') + assert response.headers["cache-control"] == "private, max-age=60" diff --git a/x2s3/client_aioboto.py b/x2s3/client_aioboto.py index 682fcd7..5ebfa2f 100644 --- a/x2s3/client_aioboto.py +++ b/x2s3/client_aioboto.py @@ -171,7 +171,9 @@ async def head_object(self, key: str): "Content-Length": str(s3_res.get("ContentLength")), # format_http_date, not strftime: %a/%b are locale-dependent "Last-Modified": format_http_date(s3_res.get("LastModified").timestamp()), - "Cache-Control": CACHE_CONTROL_PUBLIC, + # Our default only applies to objects carrying no policy of + # their own (see open_object) + "Cache-Control": s3_res.get("CacheControl") or CACHE_CONTROL_PUBLIC, } if self.proxy_etag: @@ -236,6 +238,13 @@ async def open_object(self, key: str, range_header: str = None): if "last-modified" in res_headers: headers["Last-Modified"] = res_headers["last-modified"] + if "cache-control" in res_headers: + # The origin's policy wins over our default. Re-advertising a + # no-store or private object as publicly cacheable for an hour + # would let browsers and the shared nginx cache in front of + # x2s3 hold on to content the origin said not to store. + headers["Cache-Control"] = res_headers["cache-control"] + if self.proxy_etag and "etag" in res_headers: headers["ETag"] = res_headers["etag"] From 3eb8af2fdc5a8c9950815d344ff615dcc6488a05 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 19 Aug 2026 22:03:11 -0400 Subject: [PATCH 14/19] perf: let S3 answer conditional GETs instead of refetching Revalidating a cached object opened a full upstream GetObject and threw the body away to answer 304, aborting the pooled connection with it. As soon as the new max-age expires, a viewer revalidating thousands of cached chunks cost nearly as much upstream as never having cached them. S3 evaluates IfNoneMatch/IfModifiedSince itself, so forward them and turn its 304 into ours. The dispatcher still checks validators locally, so backends that ignore the hints stay correct. The 304 carries an ETag only when proxy_etag is on, matching what the 200 would have exposed. Co-Authored-By: Claude Fable 5 --- tests/test_boto.py | 75 +++++++++++++++++++++++++++++++++++++++++- x2s3/app.py | 19 +++++++---- x2s3/client.py | 12 +++++-- x2s3/client_aioboto.py | 50 ++++++++++++++++++++++++++-- x2s3/client_file.py | 11 +++++-- 5 files changed, 154 insertions(+), 13 deletions(-) diff --git a/tests/test_boto.py b/tests/test_boto.py index 1772f01..4a26b5b 100644 --- a/tests/test_boto.py +++ b/tests/test_boto.py @@ -178,13 +178,16 @@ def test_get_object_precedence(app, s3_client): class _StubS3: """Minimal stand-in for the aiobotocore client, recording its calls.""" - def __init__(self, get_headers=None, head_response=None): + def __init__(self, get_headers=None, head_response=None, get_error=None): self.get_headers = get_headers or {} self.head_response = head_response or {} + self.get_error = get_error self.calls = [] async def get_object(self, **kwargs): self.calls.append(kwargs) + if self.get_error is not None: + raise self.get_error return {"ResponseMetadata": {"HTTPHeaders": self.get_headers}, "Body": None} @@ -253,3 +256,73 @@ def test_head_object_keeps_upstream_cache_control(): }) response = call_with_stub(stub, 'head_object', 'some/key.json') assert response.headers["cache-control"] == "private, max-age=60" + + +NOT_MODIFIED_HEADERS = {"etag": '"upstream-etag"', + "last-modified": "Fri, 26 Jul 2024 13:39:10 GMT"} + + +def _not_modified_error(): + """The ClientError real S3 raises for a conditional GET that matches.""" + from botocore.exceptions import ClientError + return ClientError({"Error": {"Code": "304", "Message": "Not Modified"}, + "ResponseMetadata": {"HTTPStatusCode": 304, + "HTTPHeaders": NOT_MODIFIED_HEADERS}}, + "GetObject") + + +def test_open_object_forwards_conditional_headers_to_s3(): + # Without this the proxy fetches the whole object upstream and throws the + # body away to answer 304, so a client revalidating thousands of cached + # chunks costs nearly as much as never having cached them. + stub = _StubS3(get_headers={"content-length": "10"}) + call_with_stub(stub, 'open_object', 'some/key.json', None, + '"client-etag"', 'Fri, 26 Jul 2024 13:39:10 GMT') + assert stub.calls[0]["IfNoneMatch"] == '"client-etag"' + assert stub.calls[0]["IfModifiedSince"] == parsedate_to_datetime( + "Fri, 26 Jul 2024 13:39:10 GMT") + + +def test_open_object_returns_304_when_s3_says_not_modified(): + stub = _StubS3(get_error=_not_modified_error()) + response = call_with_stub(stub, 'open_object', 'some/key.json', None, + '"upstream-etag"', None) + assert response.status_code == 304 + assert response.headers["last-modified"] == NOT_MODIFIED_HEADERS["last-modified"] + assert response.headers["cache-control"] == CACHE_CONTROL_PUBLIC + + +def test_304_from_s3_carries_etag_only_when_proxied(): + # proxy_etag=False exists to keep upstream ETags off the wire for backends + # whose ETags break the AWS SDK integrity check; a 304 must not leak one. + stub = _StubS3(get_error=_not_modified_error()) + hidden = call_with_stub(stub, 'open_object', 'some/key.json', None, + '"upstream-etag"', None) + assert "etag" not in hidden.headers + + stub = _StubS3(get_error=_not_modified_error()) + shown = call_with_stub(stub, 'open_object', 'some/key.json', None, + '"upstream-etag"', None, proxy_etag=True) + assert shown.headers["etag"] == NOT_MODIFIED_HEADERS["etag"] + + +def test_unparseable_if_modified_since_is_not_forwarded(): + # botocore wants a datetime; handing it a garbage string would raise, so a + # slightly-off client header must not become a 500. + stub = _StubS3(get_headers={"content-length": "10"}) + handle = call_with_stub(stub, 'open_object', 'some/key.json', None, + None, 'not a date') + assert "IfModifiedSince" not in stub.calls[0] + assert handle.status_code == 200 + + +def test_conditional_get_returns_304_against_real_s3(app, s3_client): + # End-to-end through the running proxy: the ETag the client got back must + # be answerable with a 304 rather than a second full body. + bucket = 'janelia-data-examples-with-etag' + key = 'jrc_mus_lung_covid.n5/attributes.json' + etag = s3_client.get_object(Bucket=bucket, Key=key)['ETag'] + + with pytest.raises(s3_client.exceptions.ClientError) as exc_info: + s3_client.get_object(Bucket=bucket, Key=key, IfNoneMatch=etag) + assert exc_info.value.response['ResponseMetadata']['HTTPStatusCode'] == 304 diff --git a/x2s3/app.py b/x2s3/app.py index bf9af4e..1503183 100644 --- a/x2s3/app.py +++ b/x2s3/app.py @@ -399,6 +399,13 @@ def _handle_or_denied(handle): return handle return None + async def _open(key, range_header): + """open_object with this request's conditional headers attached.""" + return await client.open_object( + key, range_header, + request.headers.get("if-none-match"), + request.headers.get("if-modified-since")) + async def get_object_or_denied(key): """GetObject with S3-style 404 masking: on unbrowseable buckets a missing key returns 403 AccessDenied so clients can't probe which @@ -408,11 +415,11 @@ async def get_object_or_denied(key): then either answers 304 or streams. The handle is closed on the 304 path — nothing else will. - ponytail: for S3 targets this means an upstream fetch is opened and - abandoned on a 304. Forward IfNoneMatch into client_aioboto if S3 - targets ever become a hot path. + Backends that evaluate the conditional headers themselves return + a 304 straight out of _open with no body transferred; the + check_not_modified call below covers the ones that don't. """ - handle = await client.open_object(key, request.headers.get("range")) + handle = await _open(key, request.headers.get("range")) denied = _handle_or_denied(handle) if denied is not None and denied.status_code != 416: return denied @@ -429,7 +436,7 @@ async def get_object_or_denied(key): and "if-none-match" not in request.headers \ and "if-modified-since" not in request.headers: return denied - handle = await client.open_object(key, None) + handle = await _open(key, None) full_denied = _handle_or_denied(handle) if full_denied is not None: return full_denied @@ -449,7 +456,7 @@ async def get_object_or_denied(key): # file that's since been overwritten would silently stitch # bytes from two versions into one corrupt chunk. handle.close() - handle = await client.open_object(key, None) + handle = await _open(key, None) denied = _handle_or_denied(handle) if denied is not None: return denied diff --git a/x2s3/client.py b/x2s3/client.py index fc7b3af..856238a 100644 --- a/x2s3/client.py +++ b/x2s3/client.py @@ -32,15 +32,23 @@ async def head_object(self, key: str): https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadObject.html """ - async def open_object(self, key: str, range_header: str = None): + async def open_object(self, key: str, range_header: str = None, + if_none_match: str = None, + if_modified_since: str = None): """ Open an object and return a handle for streaming. This performs the file/storage operation and returns an ObjectHandle containing metadata and a reference to the content, or an error Response. + The conditional headers are passed through so that backends which can + evaluate them remotely may answer 304 without transferring a body. + Doing so is optional: the dispatcher re-checks the validators against + the returned handle, so a client that ignores them is still correct, + just more expensive. + Returns: - ObjectHandle on success, or Response on error + ObjectHandle on success, or Response on error (including 304) """ def stream_object(self, handle: ObjectHandle): diff --git a/x2s3/client_aioboto.py b/x2s3/client_aioboto.py index 5ebfa2f..4c25591 100644 --- a/x2s3/client_aioboto.py +++ b/x2s3/client_aioboto.py @@ -3,6 +3,7 @@ import sys import typing from dataclasses import dataclass +from email.utils import parsedate_to_datetime from typing import Any from typing_extensions import override @@ -32,6 +33,12 @@ def close(self): self._closed = True +def _is_not_modified(e): + """True for the ClientError S3 raises when a conditional GET matches.""" + return (isinstance(e, botocore.exceptions.ClientError) + and e.response.get('ResponseMetadata', {}).get('HTTPStatusCode') == 304) + + def handle_s3_exception(e, key=None): """ Handle various cases of generic errors from the boto AWS API. """ @@ -188,8 +195,15 @@ async def head_object(self, key: str): @override - async def open_object(self, key: str, range_header: str = None): - """Open an S3 object and return a handle for streaming.""" + async def open_object(self, key: str, range_header: str = None, + if_none_match: str = None, + if_modified_since: str = None): + """Open an S3 object and return a handle for streaming. + + Conditional headers are forwarded to S3, which evaluates them itself + and answers 304 without sending a body. Without that, revalidating a + cached object means fetching it in full and discarding it. + """ real_key = key if self.bucket_prefix: real_key = os.path.join(self.bucket_prefix, key) if key else self.bucket_prefix @@ -217,6 +231,17 @@ async def open_object(self, key: str, range_header: str = None): } if range_header: get_object_params["Range"] = range_header + if if_none_match: + get_object_params["IfNoneMatch"] = if_none_match + if if_modified_since: + try: + get_object_params["IfModifiedSince"] = \ + parsedate_to_datetime(if_modified_since) + except (TypeError, ValueError): + # botocore requires a datetime, so a malformed client + # header just skips the upstream condition rather than + # turning into a 500. The dispatcher still checks locally. + pass # Call S3 get_object result = await self.client.get_object(**get_object_params) @@ -259,8 +284,29 @@ async def open_object(self, key: str, range_header: str = None): ) except Exception as e: + if _is_not_modified(e): + return self._not_modified_response(e, headers) return handle_s3_exception(e, key) + + def _not_modified_response(self, e, headers): + """Build the 304 for a conditional GET that S3 answered as unchanged. + + S3 returns the validators on its 304, so they are echoed back rather + than re-fetched. A 304 must carry no body, which is also why this + cannot go through handle_s3_exception: that renders an XML error. + """ + res_headers = e.response.get('ResponseMetadata', {}).get('HTTPHeaders', {}) + not_modified_headers = { + 'Cache-Control': res_headers.get('cache-control') + or headers['Cache-Control'], + } + if 'last-modified' in res_headers: + not_modified_headers['Last-Modified'] = res_headers['last-modified'] + if self.proxy_etag and 'etag' in res_headers: + not_modified_headers['ETag'] = res_headers['etag'] + return Response(status_code=304, headers=not_modified_headers) + @override def stream_object(self, handle: S3ObjectHandle): """Stream content from an opened S3 object handle.""" diff --git a/x2s3/client_file.py b/x2s3/client_file.py index 313994e..0d97c75 100644 --- a/x2s3/client_file.py +++ b/x2s3/client_file.py @@ -212,8 +212,15 @@ async def head_object(self, key: str): @override - async def open_object(self, key: str, range_header: str = None): - """Open a file object and return a handle for streaming.""" + async def open_object(self, key: str, range_header: str = None, + if_none_match: str = None, + if_modified_since: str = None): + """Open a file object and return a handle for streaming. + + The conditional arguments are accepted for interface parity and + ignored: opening a local file is an open plus an fstat, so there is + nothing to save by evaluating them here rather than in the dispatcher. + """ file_handle = None try: path = self._safe_path(key) From 201decb64c07264261e47d26fdd4a6be9d32766b Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 19 Aug 2026 22:03:57 -0400 Subject: [PATCH 15/19] fix: parse If-None-Match without splitting quoted ETags A quoted entity-tag may contain a comma (RFC 9110 8.8.3), and splitting the header on bare commas cut such a tag in half so it could never match: the client would revalidate forever and always get a full body. Split on the commas outside quotes instead, which leaves unquoted ETags working as before. Co-Authored-By: Claude Fable 5 --- tests/test_utils_caching.py | 21 +++++++++++++++++++++ x2s3/utils.py | 24 +++++++++++++++++++++++- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/tests/test_utils_caching.py b/tests/test_utils_caching.py index f387352..7530604 100644 --- a/tests/test_utils_caching.py +++ b/tests/test_utils_caching.py @@ -136,3 +136,24 @@ def test_star_if_none_match_matches_even_without_etag(): response = check_not_modified({"if-none-match": "*"}, {"Last-Modified": LAST_MODIFIED}) assert response is not None and response.status_code == 304 + + +def test_etag_containing_a_comma_still_matches(): + # RFC 9110 8.8.3 lets a quoted entity-tag contain a comma, so If-None-Match + # cannot be split on bare commas without cutting one in half. + etag = '"abc,def"' + headers = {"ETag": etag, "Last-Modified": LAST_MODIFIED} + assert check_not_modified({"if-none-match": etag}, headers) is not None + + +def test_comma_etag_in_a_list_still_matches(): + etag = '"abc,def"' + headers = {"ETag": etag, "Last-Modified": LAST_MODIFIED} + assert check_not_modified({"if-none-match": f'"other", {etag}'}, headers) is not None + + +def test_unquoted_etag_still_matches(): + # Guard: a quote-only parser would stop matching backends that emit + # unquoted ETags, which are likelier in the wild than comma-bearing ones. + headers = {"ETag": "abc123", "Last-Modified": LAST_MODIFIED} + assert check_not_modified({"if-none-match": "abc123"}, headers) is not None diff --git a/x2s3/utils.py b/x2s3/utils.py index 966d238..8b7dfee 100644 --- a/x2s3/utils.py +++ b/x2s3/utils.py @@ -293,9 +293,31 @@ def format_http_date(timestamp) -> str: return formatdate(timestamp, usegmt=True) +def _split_etags(if_none_match: str): + """Split an If-None-Match header into its entity-tags. + + Not a plain split(','): a quoted entity-tag may contain a comma of its own + (RFC 9110 8.8.3), and cutting one in half means it can never match, so the + client revalidates forever and always gets a full body back. Tags are kept + quoted because that is how they are compared. + """ + tags, current, quoted = [], [], False + for char in if_none_match: + if char == '"': + quoted = not quoted + current.append(char) + elif char == ',' and not quoted: + tags.append(''.join(current)) + current = [] + else: + current.append(char) + tags.append(''.join(current)) + return [tag.strip() for tag in tags if tag.strip()] + + def _etag_matches(if_none_match: str, etag: str) -> bool: """True if any entity-tag in an If-None-Match header matches ours.""" - for candidate in if_none_match.split(','): + for candidate in _split_etags(if_none_match): candidate = candidate.strip() if candidate == '*': # '*' matches any existing representation (RFC 9110 13.1.2), From 3da037e34d73965b18b7be5a6ed30ec7dc7c8c98 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 19 Aug 2026 22:04:50 -0400 Subject: [PATCH 16/19] refactor: drop the unused get_object convenience methods The dispatcher now opens and streams objects in separate steps so the validator check can see the ETag, which left these three with no callers. They are worth removing rather than leaving: each one skips the conditional-request handling entirely, so a future caller reaching for the obvious-looking helper would silently lose 304 support. Co-Authored-By: Claude Fable 5 --- x2s3/client.py | 8 -------- x2s3/client_aioboto.py | 14 -------------- x2s3/client_file.py | 13 ------------- 3 files changed, 35 deletions(-) diff --git a/x2s3/client.py b/x2s3/client.py index 856238a..be9cab8 100644 --- a/x2s3/client.py +++ b/x2s3/client.py @@ -62,14 +62,6 @@ def stream_object(self, handle: ObjectHandle): StreamingResponse that streams the object content """ - async def get_object(self, key: str, range_header: str = None): - """ - Basic interface for AWS S3's GetObject API. - https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetObject.html - - This is a convenience method that combines open_object() and stream_object(). - """ - async def list_objects_v2(self, continuation_token: str, delimiter: str, diff --git a/x2s3/client_aioboto.py b/x2s3/client_aioboto.py index 4c25591..9d1989d 100644 --- a/x2s3/client_aioboto.py +++ b/x2s3/client_aioboto.py @@ -320,20 +320,6 @@ def stream_object(self, handle: S3ObjectHandle): content_length=handle.content_length, ) - @override - async def get_object(self, key: str, range_header: str = None): - """Convenience method that combines open_object() and stream_object().""" - result = await self.open_object(key, range_header) - if isinstance(result, S3ObjectHandle): - try: - return self.stream_object(result) - except Exception: - # Ensure body is closed if stream_object fails - result.close() - raise - return result # Error response - - @override async def list_objects_v2(self, continuation_token: str, diff --git a/x2s3/client_file.py b/x2s3/client_file.py index 0d97c75..05f3603 100644 --- a/x2s3/client_file.py +++ b/x2s3/client_file.py @@ -313,19 +313,6 @@ def stream_object(self, handle: FileObjectHandle): media_type=handle.media_type ) - @override - async def get_object(self, key: str, range_header: str = None): - """Convenience method that combines open_object() and stream_object().""" - result = await self.open_object(key, range_header) - if isinstance(result, FileObjectHandle): - try: - return self.stream_object(result) - except Exception: - # Ensure file is closed if stream_object fails - result.close() - raise - return result # Error response - @override async def list_objects_v2(self, From adf8964441f77c384143f3d817f3b2ecb28d7864 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 19 Aug 2026 22:06:32 -0400 Subject: [PATCH 17/19] fix: restore the Cache-Control: no-cache cache bypass A location-level proxy_cache_bypass replaces the http-level one rather than adding to it, so every location including proxy_cache.conf (both of them) silently lost the no-cache bypass that nginx.conf set up: a client explicitly asking for a fresh copy was served nginx's stored one instead. Name both conditions together in the location, and move the map next to the other cache maps. Co-Authored-By: Claude Fable 5 --- docker/include/proxy_cache.conf | 7 ++++++- docker/include/proxy_cache_maps.conf | 15 ++++++++++++--- docker/nginx.conf | 11 ++++------- tests/test_nginx_cache_headers.py | 11 +++++++++++ 4 files changed, 33 insertions(+), 11 deletions(-) diff --git a/docker/include/proxy_cache.conf b/docker/include/proxy_cache.conf index a93ece8..8c75c0e 100644 --- a/docker/include/proxy_cache.conf +++ b/docker/include/proxy_cache.conf @@ -30,7 +30,12 @@ proxy_cache_revalidate on; # Ranged responses must still stay out of the cache: proxy_cache_key has no # Range in it, so caching a 206 would serve those bytes for every other range # of the same object. Unranged GETs (zarr.json, bucket listings) still cache. -proxy_cache_bypass $http_range; +# $cache_bypass (Cache-Control: no-cache) is repeated from proxy_cache_maps.conf +# rather than left at http level: a location-level proxy_cache_bypass replaces +# the http-level one instead of adding to it, so naming only $http_range here +# would silently drop the no-cache policy in every location that includes this +# file. +proxy_cache_bypass $http_range $cache_bypass; proxy_no_cache $http_range; proxy_set_header Range $http_range; proxy_set_header If-Range $http_if_range; diff --git a/docker/include/proxy_cache_maps.conf b/docker/include/proxy_cache_maps.conf index 5cd9a05..da917ee 100644 --- a/docker/include/proxy_cache_maps.conf +++ b/docker/include/proxy_cache_maps.conf @@ -1,6 +1,15 @@ -# Conditional-request headers, split out because map is http-context only. -# Included from nginx.conf; consumed by the proxy_set_header lines in -# proxy_cache.conf. +# Cache-related maps, split out because map is http-context only. Included from +# nginx.conf; consumed by proxy_cache.conf. + +# A client asking for a fresh copy with Cache-Control: no-cache should not be +# handed nginx's stored one. Only proxy_cache_bypass is set from this, not +# proxy_no_cache: no-cache means "revalidate before serving", so the response +# should still refresh the cache entry. +map $http_cache_control $cache_bypass { + no-cache 1; +} + +# The remaining maps restore conditional-request headers on ranged requests. # # Configuring proxy_cache makes nginx replace the client's If-None-Match and # If-Modified-Since with its own revalidation values ($upstream_cache_etag and diff --git a/docker/nginx.conf b/docker/nginx.conf index 186e656..b26001d 100644 --- a/docker/nginx.conf +++ b/docker/nginx.conf @@ -11,13 +11,10 @@ events { http { proxy_cache_path /var/cache/nginx keys_zone=mycache:512m max_size=100g levels=1:2 inactive=24h loader_sleep=10ms manager_files=4000 manager_threshold=200m manager_sleep=100ms; - map $http_cache_control $cache_bypass { - no-cache 1; - } - proxy_cache_bypass $cache_bypass; - - # map is http-context only, so the conditional-header maps consumed by - # proxy_cache.conf have to be included out here + # map is http-context only, so the maps consumed by proxy_cache.conf have + # to be included out here. The matching proxy_cache_bypass lives in + # proxy_cache.conf: a location-level one replaces an http-level one rather + # than adding to it, so both policies have to be named together there. include /etc/nginx/conf/proxy_cache_maps.conf; # Increase in-memory buffers (this assumes we have a lot of RAM) diff --git a/tests/test_nginx_cache_headers.py b/tests/test_nginx_cache_headers.py index 692d5f4..a1650a5 100644 --- a/tests/test_nginx_cache_headers.py +++ b/tests/test_nginx_cache_headers.py @@ -163,3 +163,14 @@ def test_cache_revalidation_uses_nginx_own_validator(nginx): return time.sleep(0.25) pytest.fail("cache entry never expired, so revalidation was never exercised") + + +def test_no_cache_request_bypasses_the_cache(nginx): + # nginx.conf maps Cache-Control: no-cache to a cache bypass, but a + # location-level proxy_cache_bypass replaces the http-level one rather than + # adding to it, so including proxy_cache.conf silently dropped that policy. + status, _ = request_through_nginx(nginx, "/no-cache-probe") + assert status == "MISS" + status, _ = request_through_nginx(nginx, "/no-cache-probe", + **{"Cache-Control": "no-cache"}) + assert status == "BYPASS" From 3ea9ba171d6a29ba0b2c82400c6acf405ad86d88 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Wed, 19 Aug 2026 22:07:57 -0400 Subject: [PATCH 18/19] test: pin HEAD and GET to the same caching headers The validators are assembled separately in head_object and open_object for each backend. They agree today, and nothing in the suite noticed if they stopped: a client revalidating against the pair would just get a full body forever. Guard the invariant directly rather than merging the paths, since the three sites derive it from different shapes (os.stat, the parsed boto response, and raw upstream headers). Co-Authored-By: Claude Fable 5 --- tests/test_boto.py | 11 +++++++++++ tests/test_file.py | 11 +++++++++++ 2 files changed, 22 insertions(+) diff --git a/tests/test_boto.py b/tests/test_boto.py index 4a26b5b..89c87cf 100644 --- a/tests/test_boto.py +++ b/tests/test_boto.py @@ -326,3 +326,14 @@ def test_conditional_get_returns_304_against_real_s3(app, s3_client): with pytest.raises(s3_client.exceptions.ClientError) as exc_info: s3_client.get_object(Bucket=bucket, Key=key, IfNoneMatch=etag) assert exc_info.value.response['ResponseMetadata']['HTTPStatusCode'] == 304 + + +def test_head_and_get_agree_on_caching_headers_over_s3(app, s3_client): + # Same invariant as the file client, across the other pair of code paths. + bucket = 'janelia-data-examples-with-etag' + key = 'jrc_mus_lung_covid.n5/attributes.json' + head = s3_client.head_object(Bucket=bucket, Key=key) + get = s3_client.get_object(Bucket=bucket, Key=key) + assert head['ETag'] == get['ETag'] + assert head['LastModified'] == get['LastModified'] + assert head.get('CacheControl') == get.get('CacheControl') diff --git a/tests/test_file.py b/tests/test_file.py index 3ce3cb3..06c79d0 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -491,3 +491,14 @@ def test_if_range_matching_last_modified_returns_ranged_body(app): "If-Range": full.headers['last-modified']}) assert response.status_code == 206 assert response.content == full.content[:10] + + +def test_head_and_get_agree_on_caching_headers(app): + # The validators are built separately in head_object and open_object. If + # they ever drift, a client revalidating against the pair gets a full body + # forever, and nothing else in the suite would notice. + with TestClient(app) as client: + head = client.head("/local-files/README.md") + get = client.get("/local-files/README.md") + for header in ("etag", "last-modified", "cache-control"): + assert head.headers[header] == get.headers[header], header From 2f8c3196e1af3fa68e97964c51459849fcfef8b9 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Thu, 20 Aug 2026 12:05:28 -0400 Subject: [PATCH 19/19] refactor: use the real file ETag in listings, drop calculate_etags Listings returned STATIC_ETAG, the same constant for every object, so a client that cached something it found in a listing could never revalidate it: the ETag it held matched nothing. Listings already stat each file for Size and LastModified, so returning make_file_etag costs nothing and makes listing, GET, and HEAD agree on one validator. That leaves calculate_etags with nothing to do. It only ever affected listings, it read every file in full to do it, and object responses ignored it, so it bought an expensive content hash in the one place nothing verified content. Removing it takes calc_etag and STATIC_ETAG with it. An existing config still setting the option keeps working; extra options are absorbed and ignored. Co-Authored-By: Claude Fable 5 --- docs/Config.md | 2 -- tests/test_file.py | 39 +++++++++++++++------------------------ x2s3/client_file.py | 23 ++++------------------- 3 files changed, 19 insertions(+), 45 deletions(-) diff --git a/docs/Config.md b/docs/Config.md index e0db539..7cdd671 100644 --- a/docs/Config.md +++ b/docs/Config.md @@ -19,7 +19,6 @@ client_options: max_pool_connections: 50 file: buffer_size: 65536 # 64 KB chunks for streaming - calculate_etags: false ``` ## Targets @@ -41,7 +40,6 @@ Each target may have the following properties: * *file*: Local filesystem targets. Options: * `path`: Path to the root * `buffer_size`: Size of chunks (in bytes) when streaming file content (default: 8192) - * `calculate_etags`: If true, then the etags will be calculated by hashing the content of each file. This is much more expensive and may not be needed for all use cases. ### Botocore Config Options diff --git a/tests/test_file.py b/tests/test_file.py index 06c79d0..f16f32b 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -5,7 +5,6 @@ from pydantic import HttpUrl from x2s3.app import create_app -from x2s3.client_file import STATIC_ETAG from x2s3.settings import Target, Settings from x2s3.utils import parse_xml @@ -19,14 +18,6 @@ def get_settings(): client='file', options={'path':'.'} ), - Target( - name='local-files-with-etags', - client='file', - options={ - 'path':'.', - 'calculate_etags':'true' - } - ), Target( name='hidden-files', browseable=False, @@ -79,21 +70,8 @@ def test_list_objects(app): for content in contents: etag = content.find('ETag').text assert etag.startswith('"') - assert etag==STATIC_ETAG - - -def test_list_objects_with_etags(app): - with TestClient(app) as client: - bucket_name = 'local-files-with-etags' - response = client.get(f"/{bucket_name}?list-type=2&prefix=tests/") - assert response.status_code == 200 - root = parse_xml(response.text) - assert root.tag == "ListBucketResult" - assert root.find('Name').text == bucket_name - for content in root.findall('Contents'): - etag = content.find('ETag').text - assert etag.startswith('"') - assert etag!=STATIC_ETAG + # mtime-size, not a constant: see make_file_etag + assert etag != '"11111111111111111111111111111111"' def test_list_objects_delimiter(app): @@ -502,3 +480,16 @@ def test_head_and_get_agree_on_caching_headers(app): get = client.get("/local-files/README.md") for header in ("etag", "last-modified", "cache-control"): assert head.headers[header] == get.headers[header], header + + +def test_listing_etag_matches_get_etag(app): + # A client that caches an object it found in a listing must be able to + # revalidate with that listing's ETag. A constant ETag guaranteed a miss. + with TestClient(app) as client: + listing = client.get("/local-files?list-type=2&prefix=tests/&max-keys=1") + entry = parse_xml(listing.text).find('Contents') + key, listed = entry.find('Key').text, entry.find('ETag').text + + assert client.get(f"/local-files/{key}").headers['etag'] == listed + assert client.get(f"/local-files/{key}", + headers={"If-None-Match": listed}).status_code == 304 diff --git a/x2s3/client_file.py b/x2s3/client_file.py index 05f3603..0ce78b0 100644 --- a/x2s3/client_file.py +++ b/x2s3/client_file.py @@ -1,7 +1,6 @@ import os import sys from dataclasses import dataclass -from hashlib import md5 from pathlib import Path from typing import BinaryIO, Optional, Tuple from typing_extensions import override @@ -34,8 +33,6 @@ def close(self): self.file_handle = None -STATIC_ETAG = '"11111111111111111111111111111111"' - def handle_exception(e, key=None): """ Handle various cases of generic errors. """ @@ -150,22 +147,12 @@ def file_iterator(handle: FileObjectHandle, buffer_size: int = DEFAULT_BUFFER_SI handle.close() -# From https://teppen.io/2018/10/23/aws_s3_verify_etags/ -def calc_etag(inputfile, partsize): - md5_digests = [] - with open(inputfile, 'rb') as f: - for chunk in iter(lambda: f.read(partsize), b''): - md5_digests.append(md5(chunk).digest()) - return md5(b''.join(md5_digests)).hexdigest() + '-' + str(len(md5_digests)) - - class FileProxyClient(ProxyClient): def __init__(self, proxy_kwargs, **kwargs): self.proxy_kwargs = proxy_kwargs or {} self.target_name = self.proxy_kwargs['target_name'] self.root_path = str(Path(kwargs['path']).resolve()) - self.calculate_etags = kwargs.get('calculate_etags', False) self.buffer_size = kwargs.get('buffer_size', DEFAULT_BUFFER_SIZE) def _safe_path(self, key: str) -> Optional[str]: @@ -408,15 +395,13 @@ def walk_path(self, path, continuation_token, delimiter, max_keys): stats = os.stat(file_path) file_size = stats.st_size - etag = STATIC_ETAG - if self.calculate_etags: - # This is VERY slow because it needs to read every file - etag = f'"{calc_etag(file_path, 8388608)}"' - contents.append({ 'Key': key, 'Size': str(file_size), - 'ETag': etag, + # Same validator head_object/open_object return, so a + # client can revalidate what it found here and get a + # 304. Free: the stat above already has both fields. + 'ETag': make_file_etag(stats.st_mtime, file_size), 'LastModified': format_timestamp_s3(stats.st_mtime), 'StorageClass': 'STANDARD' })