Skip to content

HTTP caching: Cache-Control, ETag, and conditional requests - #27

Open
krokicki wants to merge 19 commits into
mainfrom
cache-headers
Open

HTTP caching: Cache-Control, ETag, and conditional requests#27
krokicki wants to merge 19 commits into
mainfrom
cache-headers

Conversation

@krokicki

@krokicki krokicki commented Aug 20, 2026

Copy link
Copy Markdown
Member

Makes x2s3 responses cacheable and revalidatable, so viewers like Neuroglancer and N5 Viewer stop refetching bytes they already have.

What this adds

  • Cache-Control, ETag, and an RFC 7231 Last-Modified on file- and S3-backed objects.
  • Conditional requests: If-None-Match / If-Modified-Since are answered with 304 on GET and HEAD.
  • If-Range handling, so a resumed download against a file that changed underneath gets the full new representation instead of two versions stitched together.
  • nginx is no longer told to hide Cache-Control, so the policy reaches browsers and shared caches.

Review fixes included

A review of the branch turned up several issues, each fixed with a failing test first:

Fix Problem
Conditionals before 416 An unsatisfiable Range short-circuited conditional handling, so a client resuming a download of a file that had shrunk got a permanent 416 instead of a 304 or the full body (RFC 9110 13.2.2).
nginx conditional headers proxy_cache makes nginx replace the client's If-None-Match/If-Modified-Since with its own revalidation values, which made the new 304 support unreachable behind the production nginx. Restored for cache-bypassed (ranged) requests only — forwarding them on cached requests would let a client's ETag answer nginx's revalidation and mark a stale entry fresh.
Origin Cache-Control Every S3-backed response was stamped public, max-age=3600, overriding an origin no-store or private. The default now applies only to objects carrying no policy.
S3 conditional forwarding Revalidation opened a full upstream GetObject and discarded the body, so a viewer revalidating thousands of chunks cost nearly as much as never having cached them. IfNoneMatch/IfModifiedSince are now forwarded and S3's 304 translated.
Locale-safe Last-Modified strftime('%a, %d %b %Y ...') expands month names using the process locale, producing an unparseable date under a non-C locale and silently killing revalidation.
If-None-Match: * Never matched on targets that don't proxy ETags (the proxy_etag=False default), because the empty-ETag guard ran before the * check.
ETag parsing If-None-Match was split on bare commas, cutting quoted ETags that legally contain one.
no-cache bypass Pre-existing: a location-level proxy_cache_bypass replaces the http-level one rather than adding to it, so including proxy_cache.conf silently dropped the Cache-Control: no-cache bypass in both locations.

Testing

134 tests pass, including the network tests against the real janelia-data-examples bucket.

New coverage includes tests/test_nginx_cache_headers.py, which runs real nginx in front of an echo upstream and asserts on the headers that actually arrive — nginx's header substitution is invisible from either end alone. It skips automatically when no nginx binary is present.

Verified beyond the suite:

  • End-to-end through real nginx + x2s3, a ranged conditional GET returns 206 before the nginx fix and 304 after.
  • Real S3 evaluates conditionals before Range (unsatisfiable range alone → 416; with a matching validator → 304), matching the ordering now implemented for the file backend, so both backends agree.
  • The production nginx.conf still passes nginx -t.
  • The active fileglancer repo consumes FileProxyClient.open_object positionally; the added parameters are optional and appended, so that call still binds (checked directly).

@StephanPreibisch @JaneliaSciComp/fileglancer @neomorphic

krokicki and others added 19 commits August 18, 2026 15:54
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.
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).
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.
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant