Copilot commented on code in PR #12852:
URL: https://github.com/apache/trafficserver/pull/12852#discussion_r2770154369
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -2942,14 +2954,26 @@ HttpTransact::HandleCacheOpenReadHit(State *s)
HttpCacheSM &cache_sm = s->state_machine->get_cache_sm();
TxnDbg(dbg_ctl_http_trans, "CacheOpenRead --- HIT-FRESH read while write
%d", cache_sm.is_readwhilewrite_inprogress());
- if (cache_sm.is_readwhilewrite_inprogress())
+ if (cache_sm.is_readwhilewrite_inprogress()) {
SET_VIA_STRING(VIA_CACHE_RESULT, VIA_IN_CACHE_RWW_HIT);
+ }
+
+ // If serving stale due to write lock failure (actions 2, 3, or 6), adjust
VIA to reflect stale serving.
+ // This ensures correct statistics attribution (cache_hit_stale_served
instead of cache_hit_fresh).
+ if (s->serving_stale_due_to_write_lock) {
+ TxnDbg(dbg_ctl_http_trans, "Serving stale due to write lock failure,
adjusting VIA for statistics");
+ SET_VIA_STRING(VIA_CACHE_RESULT, VIA_IN_CACHE_STALE);
Review Comment:
When serving stale due to write lock failure in the READ_RETRY path,
VIA_DETAIL_CACHE_LOOKUP is not being set to VIA_DETAIL_MISS_EXPIRED. This
creates an inconsistency with other HIT_STALE code paths. Compare with
HandleCacheOpenReadHitFreshness (lines 2602-2605) which sets both
VIA_DETAIL_CACHE_LOOKUP to VIA_DETAIL_MISS_EXPIRED and VIA_CACHE_RESULT to
VIA_IN_CACHE_STALE when cache_lookup_result is HIT_STALE. For consistency, add
SET_VIA_STRING for VIA_DETAIL_CACHE_LOOKUP here as well.
```suggestion
SET_VIA_STRING(VIA_CACHE_RESULT, VIA_IN_CACHE_STALE);
SET_VIA_STRING(VIA_DETAIL_CACHE_LOOKUP, VIA_DETAIL_MISS_EXPIRED);
```
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3271,20 +3384,50 @@ HttpTransact::HandleCacheOpenReadMiss(State *s)
} else if (s->api_server_response_no_store) { // plugin may have decided not
to cache the response
s->cache_info.action = CacheAction_t::NO_ACTION;
} else if (s->cache_info.write_lock_state == CacheWriteLock_t::READ_RETRY) {
- // We've looped back around due to failing to read during READ_RETRY mode.
- // Don't attempt another cache write - just proxy to origin without
caching.
+ // READ_RETRY cache read failed (no object found). If there was a stale
object,
+ // handle_cache_write_lock would have called HandleCacheOpenReadHit, not
us.
+ // Don't attempt cache write - just proxy to origin without caching.
TxnDbg(dbg_ctl_http_trans, "READ_RETRY cache read failed, bypassing
cache");
s->cache_info.action = CacheAction_t::NO_ACTION;
} else {
HttpTransact::set_cache_prepare_write_action_for_new_request(s);
}
- ///////////////////////////////////////////////////////////////
- // a normal miss would try to fetch the document from the //
- // origin server, unless the origin server isn't resolvable, //
- // but if "CacheControl: only-if-cached" is set, then we are //
- // supposed to send a 504 (GATEWAY TIMEOUT) response. //
- ///////////////////////////////////////////////////////////////
+ // If the CACHE_LOOKUP_COMPLETE hook was deferred, decide whether to fire it
now.
+ // Only fire when we're at a terminal state for cache lookup:
+ // - NO_ACTION: Not attempting write lock, MISS is the final result
+ // - READ_RETRY: Already retried and still missed, MISS is the final result
+ //
+ // Don't fire yet if action == PREPARE_TO_WRITE and write_lock_state !=
READ_RETRY,
+ // because we're about to attempt the write lock and may find content on
retry.
+ // In that case, the hook will fire later:
+ // - If READ_RETRY finds HIT: HandleCacheOpenReadHitFreshness fires with HIT
Review Comment:
The comment mentions "HandleCacheOpenReadHitFreshness fires with HIT" but
this is inaccurate. In the READ_RETRY path (handle_cache_write_lock lines
3259-3280), when a HIT is found, the code fires the CACHE_LOOKUP_COMPLETE hook
and then calls HandleCacheOpenReadHit directly, bypassing
HandleCacheOpenReadHitFreshness. Consider updating this comment to say
"handle_cache_write_lock fires CACHE_LOOKUP_COMPLETE with HIT" for accuracy.
```suggestion
// - If READ_RETRY finds HIT: handle_cache_write_lock fires
CACHE_LOOKUP_COMPLETE with HIT
```
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3207,21 +3233,108 @@ HttpTransact::handle_cache_write_lock(State *s)
}
if (s->cache_info.write_lock_state == CacheWriteLock_t::READ_RETRY) {
- TxnDbg(dbg_ctl_http_error, "calling hdr_info.server_request.destroy");
+ TxnDbg(dbg_ctl_http_trans, "READ_RETRY: destroying server_request before
cache hit handling");
s->hdr_info.server_request.destroy();
- HandleCacheOpenReadHitFreshness(s);
- } else {
- StateMachineAction_t next;
- next = how_to_open_connection(s);
- if (next == StateMachineAction_t::ORIGIN_SERVER_OPEN || next ==
StateMachineAction_t::ORIGIN_SERVER_RAW_OPEN) {
- s->next_action = next;
- TRANSACT_RETURN(next, nullptr);
+
+ // For READ_RETRY with cached object, evaluate actual freshness to decide
the path.
+ // If the initial lookup was a MISS, cache_lookup_complete_deferred is
true and we
+ // need to fire the hook here with the final result. If the initial lookup
was a
+ // HIT_STALE (revalidation case), the hook already fired and deferred is
false.
+ CacheHTTPInfo *obj = s->cache_info.object_read;
+ if (obj != nullptr) {
+ // Evaluate actual document freshness - don't let STALE_ON_REVALIDATE
short-circuit this
+ // by temporarily clearing the flag. We need to know if the object is
truly fresh or stale.
+ MgmtByte saved_action = s->cache_open_write_fail_action;
+ s->cache_open_write_fail_action =
static_cast<MgmtByte>(CacheOpenWriteFailAction_t::READ_RETRY);
+ Freshness_t freshness = what_is_document_freshness(s,
&s->hdr_info.client_request, obj->response_get());
+ s->cache_open_write_fail_action = saved_action;
+
+ if (freshness == Freshness_t::FRESH || freshness ==
Freshness_t::WARNING) {
+ // Object is fresh - serve it from cache for both action 5 and 6.
+ // This is the main benefit of request collapsing: we found a valid
cached object.
+ TxnDbg(dbg_ctl_http_trans, "READ_RETRY: found fresh object, serving
from cache");
+ s->cache_lookup_result =
+ (freshness == Freshness_t::FRESH) ? CacheLookupResult_t::HIT_FRESH :
CacheLookupResult_t::HIT_WARNING;
+
+ if (s->cache_lookup_complete_deferred) {
+ s->cache_lookup_complete_deferred = false;
+ TRANSACT_RETURN(StateMachineAction_t::API_CACHE_LOOKUP_COMPLETE,
HandleCacheOpenReadHit);
+ }
+ HandleCacheOpenReadHit(s);
Review Comment:
When the READ_RETRY path finds a cached object and directly calls
HandleCacheOpenReadHit (lines 3263, 3280), the request_sent_time and
response_received_time are not being set from the cached object. These times
are set to UNDEFINED_TIME at line 3186 but never restored from
obj->request_sent_time_get() and obj->response_received_time_get(). This
differs from the normal path through HandleCacheOpenReadHitFreshness (lines
2554-2563) where these times are properly set. This could affect freshness
calculations and Age header generation. Consider setting these times before
calling HandleCacheOpenReadHit, similar to what HandleCacheOpenReadHitFreshness
does at lines 2554-2563.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
