Copilot commented on code in PR #12852:
URL: https://github.com/apache/trafficserver/pull/12852#discussion_r2761474507
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3270,21 +3316,53 @@ HttpTransact::HandleCacheOpenReadMiss(State *s)
s->cache_info.action = CacheAction_t::NO_ACTION;
} 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) {
+ } else if (s->cache_info.write_lock_state == CacheWriteLock_t::READ_RETRY &&
!s->serving_stale_due_to_write_lock) {
// 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.
- TxnDbg(dbg_ctl_http_trans, "READ_RETRY cache read failed, bypassing
cache");
+ // Fire the deferred CACHE_LOOKUP_COMPLETE hook with MISS result so plugins
+ // see the final cache lookup status before we go to origin.
+ // NOTE: Only fire if serving_stale_due_to_write_lock is false. If true,
we already
+ // found a stale object and the hook was fired from
HandleCacheOpenReadHitFreshness.
+ // We're here because authentication (MUST_PROXY) requires going to origin
anyway.
+ TxnDbg(dbg_ctl_http_trans, "READ_RETRY cache read failed, firing deferred
CACHE_LOOKUP_COMPLETE with MISS");
+ s->cache_info.action = CacheAction_t::NO_ACTION;
+ s->cache_lookup_result = CacheLookupResult_t::MISS;
+ TRANSACT_RETURN(StateMachineAction_t::API_CACHE_LOOKUP_COMPLETE,
HandleCacheOpenReadMissGoToOrigin);
+ } else if (s->cache_info.write_lock_state == CacheWriteLock_t::READ_RETRY) {
+ // READ_RETRY with serving_stale_due_to_write_lock = true means we found
stale content
+ // but can't serve it (e.g., MUST_PROXY auth). Hook already fired, just go
to origin.
+ TxnDbg(dbg_ctl_http_trans, "READ_RETRY stale found but requires origin,
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 action 5/6 configured, we set NO_ACTION, and we're not in READ_RETRY,
+ // fire the deferred CACHE_LOOKUP_COMPLETE hook now with MISS.
+ // We won't enter READ_RETRY since we're not caching, so fire the hook now.
+ if (s->cache_info.action == CacheAction_t::NO_ACTION &&
s->cache_info.write_lock_state != CacheWriteLock_t::READ_RETRY &&
+ (s->txn_conf->cache_open_write_fail_action ==
static_cast<MgmtByte>(CacheOpenWriteFailAction_t::READ_RETRY) ||
+ s->txn_conf->cache_open_write_fail_action ==
+
static_cast<MgmtByte>(CacheOpenWriteFailAction_t::READ_RETRY_STALE_ON_REVALIDATE)))
{
+ TxnDbg(dbg_ctl_http_trans, "Action 5/6 NO_ACTION path, firing deferred
CACHE_LOOKUP_COMPLETE with MISS");
Review Comment:
In the action 5/6 path, CACHE_LOOKUP_COMPLETE is only fired here when
cache_info.action == NO_ACTION. For a normal cacheable miss (PREPARE_TO_WRITE /
PREPARE_TO_UPDATE), this function proceeds to origin without ever returning
API_CACHE_LOOKUP_COMPLETE, and state_cache_open_read no longer calls
setup_cache_lookup_complete_api for action 5/6. This appears to drop
CACHE_LOOKUP_COMPLETE entirely for regular misses under action 5/6. Ensure
API_CACHE_LOOKUP_COMPLETE still fires once per transaction for cache misses
(e.g., fire it when write_lock_state != READ_RETRY, regardless of whether
caching is enabled for the request).
```suggestion
// If action 5/6 configured and we're not in READ_RETRY, fire the deferred
// CACHE_LOOKUP_COMPLETE hook now with MISS. For cache misses under action
5/6,
// we may or may not be caching this transaction (cache_info.action could
be
// NO_ACTION or a prepare-to-write/update action). Regardless, we want the
// hook to fire once per transaction for misses before proceeding to
origin.
if (s->cache_info.write_lock_state != CacheWriteLock_t::READ_RETRY &&
(s->txn_conf->cache_open_write_fail_action ==
static_cast<MgmtByte>(CacheOpenWriteFailAction_t::READ_RETRY) ||
s->txn_conf->cache_open_write_fail_action ==
static_cast<MgmtByte>(CacheOpenWriteFailAction_t::READ_RETRY_STALE_ON_REVALIDATE)))
{
TxnDbg(dbg_ctl_http_trans, "Action 5/6 path, firing deferred
CACHE_LOOKUP_COMPLETE with MISS");
```
##########
src/proxy/http/HttpSM.cc:
##########
@@ -2663,7 +2666,20 @@ HttpSM::state_cache_open_read(int event, void *data)
ink_assert(t_state.transact_return_point == nullptr);
t_state.transact_return_point = HttpTransact::HandleCacheOpenRead;
- setup_cache_lookup_complete_api();
+
+ // For READ_RETRY actions (5 and 6), skip the CACHE_LOOKUP_COMPLETE hook
now.
+ // The hook will fire later with the final result: HIT if stale content is
found
+ // during retry, or MISS if nothing is found (from
HandleCacheOpenReadMiss).
+ // This ensures plugins see only the final cache lookup result, avoiding
issues
+ // like stats double-counting and duplicate hook registrations.
+ if (t_state.txn_conf->cache_open_write_fail_action ==
static_cast<MgmtByte>(CacheOpenWriteFailAction_t::READ_RETRY) ||
+ t_state.txn_conf->cache_open_write_fail_action ==
+
static_cast<MgmtByte>(CacheOpenWriteFailAction_t::READ_RETRY_STALE_ON_REVALIDATE))
{
+ SMDbg(dbg_ctl_http, "READ_RETRY configured, deferring
CACHE_LOOKUP_COMPLETE hook");
Review Comment:
Deferring CACHE_LOOKUP_COMPLETE here based only on config value (action 5/6)
causes the hook to be skipped for *all* cache open_read failures, including the
normal cache-miss path (write_lock_state == INIT). In that normal miss case,
HttpTransact::HandleCacheOpenReadMiss currently does not always re-fire
API_CACHE_LOOKUP_COMPLETE, so plugins may never see CACHE_LOOKUP_COMPLETE for
misses when action 5/6 is configured. Consider deferring the hook only when we
are actually in the READ_RETRY loop (e.g., when cache_info.write_lock_state ==
READ_RETRY), and keep the original immediate hook behavior for ordinary misses.
```suggestion
// For READ_RETRY actions (5 and 6), skip the CACHE_LOOKUP_COMPLETE hook
only
// when we are actually in the READ_RETRY loop. For ordinary cache misses
// (e.g. write_lock_state == INIT), keep the original immediate hook
behavior
// so plugins always see CACHE_LOOKUP_COMPLETE for misses.
if ((t_state.txn_conf->cache_open_write_fail_action ==
static_cast<MgmtByte>(CacheOpenWriteFailAction_t::READ_RETRY) ||
t_state.txn_conf->cache_open_write_fail_action ==
static_cast<MgmtByte>(CacheOpenWriteFailAction_t::READ_RETRY_STALE_ON_REVALIDATE))
&&
t_state.cache_info.write_lock_state ==
HttpTransact::CacheWriteLock_t::READ_RETRY) {
SMDbg(dbg_ctl_http, "READ_RETRY configured and in READ_RETRY state,
deferring CACHE_LOOKUP_COMPLETE hook");
```
--
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]
