2091196 - fetch_product_versions is not cached for the request: buglist.cgi makes 42 memcached lookups, and a new HTTP request on every call when the product-details endpoint fails

Status: ASSIGNED

(bugzilla.mozilla.org :: Bug Creation/Editing, enhancement)

Description

[David Lawrence [:dkl]]

Bugzilla::Util::fetch_product_versions($product) fetches release versions from the product_details_endpoint and caches them in memcached for a day. It's meant to also keep the result for the current request in request_cache, but it only does that after a successful HTTP fetch:

my $versions = Bugzilla->request_cache->{$key}
  || Bugzilla->memcached->get_data({key => $key});

unless ($versions) {
  ...   # HTTP fetch; request_cache is only set here, on success
}

That causes two problems:

1. A memcached hit is never saved for the request. Every later call in the same request goes back to memcached.
2. A failed fetch is never saved at all. When the endpoint returns an error or is unreachable, every call makes a new HTTP request.

This matters because some callers make many calls per request. The BMO extension's tf_buglist_columns hook builds column aliases for each tracking/status flag type and product channel (2 × 21), calling fetch_product_versions through _get_search_param_name each time. That's 42 calls on every buglist.cgi page.

mojo_user_agent allows 5 s to connect and 30 s per request. So if the endpoint hangs in production, one buglist page could make up to 42 slow requests in a row.

A smaller source of repeated memcached reads is the BMO extension's cf_hidden_in_product. On show_bug.cgi it looks fields up with Bugzilla::Field->new({name => ..., cache => 1}). A memcached lookup by name costs two round trips (name → id, then id → data), even though Bugzilla->fields already has every field loaded for the request.

Measured

Local dev stack, Devel::NYTProf, same data before and after:

Page	Memcached gets	HTTP requests to the endpoint	Time in fetch_product_versions	Request time
/buglist.cgi?product=Firefox before	54	18	57 ms	148 ms
/buglist.cgi?product=Firefox after	14	1	4.3 ms	79 ms
/show_bug.cgi?id=1 before	59	0	0.8 ms	166 ms
/show_bug.cgi?id=1 after	27	0	0.3 ms	154 ms

In the dev environment the endpoint returns 404, so the buglist numbers show the failing-endpoint case. Each failure also logs ERROR: Unable to retrieve product versions: 404: Not Found, which floods the dev logs.

When the endpoint is working, the fix still removes about 40 memcached round trips per buglist page.

Steps to reproduce

1. Start the dev stack with USE_NYTPROF=1.
2. Load /buglist.cgi?product=Firefox&bug_status=__all__.
3. In the profile, Bugzilla::Util::fetch_product_versions has 42 calls, and Mojo::UserAgent::get has one call per failed product lookup.

Expected

Each product's versions should be looked up at most once per request, whether the result comes from memcached, from the endpoint, or the lookup fails.

Proposed fix

1. fetch_product_versions: check request_cache with exists, and save the result there however it was obtained (memcached hit, successful fetch, or failure/undef). Then each product is looked up at most once per request. The next request still checks memcached and retries the endpoint as before, so a recovered endpoint is picked up straight away.
2. cf_hidden_in_product: read the field from Bugzilla->fields({by_name => 1}), which is already loaded for the request, and fall back to Bugzilla::Field->new({name => ..., cache => 1}) if it isn't found. The fallback keeps the existing case-insensitive name match.
   A new test, t/util-fetch-product-versions.t, covers the failing endpoint, the working endpoint, a memcached hit on a later request, separate caching per product, and no endpoint configured.
   Trade-off and possible follow-up
   Failures are only remembered for one request. During a long outage of the product-details endpoint, each page load still makes one attempt per product. Caching failures in memcached for a short time (for example 1–5 minutes) would protect better, but a brief error could then hide versions for that long. That's left out of this patch.

I added one line under Measured saying where the numbers come from (local dev stack, NYTProf, same data), so they make sense