Skip to content

Fix 92x compatibility cache revalidation - #13611

Draft
traeak wants to merge 7 commits into
apache:masterfrom
traeak:92x_compat
Draft

traeak wants to merge 7 commits into
apache:masterfrom
traeak:92x_compat

Conversation

@traeak

@traeak traeak commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Follow on to #12271 and #12283

proxy.config.http.cache.try_compat_key_read could find an object stored
under 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.

  • Revalidate compatibility-key objects without conditional headers, so the
    origin returns a full response and the existing write path migrates the
    object to the current key.
  • Reproduce the real 9.2 key, and skip the second lookup where the two keys
    provably converge.
  • Leave the legacy copy to age out rather than deleting it: nothing at the
    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.

@traeak traeak self-assigned this Sep 1, 2026
@traeak traeak added the Cache label Sep 1, 2026
@traeak
traeak requested a lite review from Copilot September 1, 2026 17:42

This comment was marked as outdated.

@traeak
traeak marked this pull request as ready for review September 1, 2026 19:03
Copilot AI review requested due to automatic review settings September 1, 2026 19:03

This comment was marked as resolved.

Copilot AI review requested due to automatic review settings September 1, 2026 20:26

This comment was marked as outdated.

Copilot AI review requested due to automatic review settings September 1, 2026 21:24

This comment was marked as outdated.

Copilot AI review requested due to automatic review settings September 1, 2026 21:58

This comment was marked as outdated.

@traeak
traeak marked this pull request as draft September 2, 2026 15:43
@traeak
traeak requested a lite review from Copilot September 8, 2026 12:47

This comment was marked as resolved.

@traeak
traeak requested a lite review from Copilot September 8, 2026 13:12

This comment was marked as resolved.

@traeak
traeak requested a lite review from Copilot September 8, 2026 15:32

This comment was marked as resolved.

@traeak
traeak requested a lite review from Copilot September 8, 2026 16:42

This comment was marked as resolved.

@traeak
traeak requested a lite review from Copilot September 14, 2026 22:45
Copilot stopped reviewing on behalf of traeak due to an error September 14, 2026 23:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Comment thread include/proxy/hdrs/URL.h
Comment on lines +503 to +506
URLImpl::has_path_params() const noexcept
{
return m_ptr_path != nullptr && memchr(m_ptr_path, ';', m_len_path) != nullptr;
}
Comment on lines +2733 to +2741
if (s->cache_lookup_result == HttpTransact::CacheLookupResult_t::HIT_STALE && s->state_machine != nullptr &&
CompatCacheKey::is_legacy(s->state_machine->compatibility_cache_lookup) &&
(s->method == HTTP_WKSIDX_GET || s->method == HTTP_WKSIDX_HEAD)) {
TxnDbg(dbg_ctl_http_seq, "Stale under the compatibility key, treating as a miss");
s->cache_info.object_read = nullptr;
s->cache_lookup_result = HttpTransact::CacheLookupResult_t::MISS;
s->cache_lookup_complete_deferred = false;
TRANSACT_RETURN(StateMachineAction_t::API_CACHE_LOOKUP_COMPLETE, HttpTransact::HandleCacheOpenReadMiss);
}
Comment on lines +881 to +882
url.create(heap.get());
REQUIRE(url.parse(text, strlen(text)) == ParseResult::DONE);
Comment thread src/proxy/http/HttpSM.cc
Comment on lines +5373 to +5374
// the purge.
if (t_state.http_config_param->cache_try_compat_key_read) {
@masaori335

Copy link
Copy Markdown
Contributor

This is more complicated than I thought in the beginning. I'd suggest fix the cache key bug with ; in path separately first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants