Fix 92x compatibility cache revalidation - #13611
Draft
traeak wants to merge 5 commits into
Draft
Conversation
traeak
marked this pull request as draft
September 2, 2026 15:43
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Comment on lines
+2803
to
+2814
| Two costs come with enabling this. Every cache miss performs a second | ||
| lookup, so a tier with a low hit ratio roughly doubles its cache lookup | ||
| load for the duration. And an object found under the previous key is | ||
| revalidated *without* conditional headers, because a ``304`` cannot be | ||
| applied to it: the write that would carry the update is a create under the | ||
| new key rather than an update of the old one. The origin therefore returns | ||
| the full response, which is stored under the new key. The copy under the | ||
| previous key is left in place to age out on its own, since nothing reports | ||
| that the new object reached disk; it stops being read as soon as the new key | ||
| resolves, so both keys briefly hold the object. Each object pays this once, | ||
| but on a large cache the aggregate is a bandwidth event worth sizing before | ||
| enabling the setting in production. |
Comment on lines
+2589
to
+2599
| if (s->state_machine != nullptr && should_use_compatibility_cache_key(s->state_machine->compatibility_cache_lookup)) { | ||
| // build_request() already strips the client's conditionals for a request it | ||
| // expects to cache, but keeps them when the request does not look cacheable | ||
| // or when cache_when_to_revalidate is 4. Either way the origin could answer | ||
| // 304, so drop them here too. The client still gets its 304: a conditional | ||
| // client request is matched against the full response in | ||
| // handle_cache_operation_on_forward_server_response(). | ||
| TxnDbg(dbg_ctl_http_trans, "compatibility key hit, revalidating without conditional headers"); | ||
| HttpTransactHeaders::remove_conditional_headers(&s->hdr_info.server_request); | ||
| return; | ||
| } |
Comment on lines
+182
to
+196
| /// Whether this lookup addresses the cache with the previous (9.2) key. | ||
| inline bool | ||
| should_use_compatibility_cache_key(CompatibilityCacheLookup lookup) | ||
| { | ||
| return lookup == CompatibilityCacheLookup::COMPAT_CACHE_LOOKUP_92; | ||
| } | ||
|
|
||
| /// The object info to hand to a cache write, which a compatibility read must not | ||
| /// carry: it belongs to the legacy key and would turn the write into an update | ||
| /// of a vector the canonical key does not have. | ||
| inline CacheHTTPInfo * | ||
| cache_write_info_for_lookup(CompatibilityCacheLookup lookup, CacheHTTPInfo *object_read_info) | ||
| { | ||
| return should_use_compatibility_cache_key(lookup) ? nullptr : object_read_info; | ||
| } |
Comment on lines
+5286
to
+5296
| // The URL this transaction looks up in the cache. A redirect follow looks up the | ||
| // redirected URL rather than the original, unless the transaction is configured | ||
| // to keep the original cache key. | ||
| URL * | ||
| HttpSM::cache_lookup_url() | ||
| { | ||
| if (t_state.redirect_info.redirect_in_process && !t_state.txn_conf->redirect_use_orig_cache_key) { | ||
| return t_state.hdr_info.client_request.url_get(); | ||
| } | ||
| return t_state.cache_info.lookup_url; | ||
| } |
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.
Follow on to #12271 and #12283
proxy.config.http.cache.try_compat_key_readcould find an object storedunder the ATS 9.2 cache key but never do anything useful with it. The write
that would carry a revalidation or an update is a create on the current key
rather than an update of the legacy vector, so the cache silently discarded
it — 304s, header updates and negative revalidating all became no-ops that
still reported success. Separately, the 9.2 key was reproduced incorrectly
for any path containing
;, so those objects were never found at all.origin returns a full response and the existing write path migrates the
object to the current key.
provably converge.
state-machine layer reports that the new object reached disk, so deleting
on the tunnel's write-complete event loses the object whenever the write
is later rejected.
NOTE: As part of cache migration, if a 92x header is detected any IMS headers to parent are stripped and the asset is freshly pulled from parent. This will cause an increase in parent bandwidth as stale objects will be repulled instead of refreshed.