HTTP caching: Cache-Control, ETag, and conditional requests - #27
Open
krokicki wants to merge 19 commits into
Open
HTTP caching: Cache-Control, ETag, and conditional requests#27krokicki wants to merge 19 commits into
krokicki wants to merge 19 commits into
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 7231Last-Modifiedon file- and S3-backed objects.If-None-Match/If-Modified-Sinceare answered with304on GET and HEAD.If-Rangehandling, so a resumed download against a file that changed underneath gets the full new representation instead of two versions stitched together.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:
416Rangeshort-circuited conditional handling, so a client resuming a download of a file that had shrunk got a permanent416instead of a304or the full body (RFC 9110 13.2.2).proxy_cachemakes nginx replace the client'sIf-None-Match/If-Modified-Sincewith its own revalidation values, which made the new304support 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.Cache-Controlpublic, max-age=3600, overriding an originno-storeorprivate. The default now applies only to objects carrying no policy.GetObjectand discarded the body, so a viewer revalidating thousands of chunks cost nearly as much as never having cached them.IfNoneMatch/IfModifiedSinceare now forwarded and S3's304translated.Last-Modifiedstrftime('%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: *proxy_etag=Falsedefault), because the empty-ETag guard ran before the*check.If-None-Matchwas split on bare commas, cutting quoted ETags that legally contain one.no-cachebypassproxy_cache_bypassreplaces the http-level one rather than adding to it, so includingproxy_cache.confsilently dropped theCache-Control: no-cachebypass in both locations.Testing
134 tests pass, including the network tests against the real
janelia-data-examplesbucket.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:
206before the nginx fix and304after.Range(unsatisfiable range alone →416; with a matching validator →304), matching the ordering now implemented for the file backend, so both backends agree.nginx.confstill passesnginx -t.fileglancerrepo consumesFileProxyClient.open_objectpositionally; the added parameters are optional and appended, so that call still binds (checked directly).@StephanPreibisch @JaneliaSciComp/fileglancer @neomorphic