diff --git a/extensions/GitHubPullRequests/lib/API/V1/PullRequests.pm b/extensions/GitHubPullRequests/lib/API/V1/PullRequests.pm index 3f706bcd21..f7b1f582fe 100644 --- a/extensions/GitHubPullRequests/lib/API/V1/PullRequests.pm +++ b/extensions/GitHubPullRequests/lib/API/V1/PullRequests.pm @@ -115,14 +115,36 @@ sub _fetch_pull_request { sortkey => int($pr_number), }; - # Return a cached summary if we have a fresh one. - my $cache_key = "github_pr." . $url; + my $reviews_url = $api_url . '/reviews?per_page=' . GITHUB_REVIEWS_PER_PAGE; + + # Look up the cached wrapper. A new key prefix (.v2.) is used so that any + # pre-existing entries from the old bare-$pr_data format are ignored and + # simply expire on their own - no migration or shape-sniffing needed. + my $cache_key = "github_pr.v2." . $url; my $cached = Bugzilla->memcached->get_data({key => $cache_key}); - return $cached if defined $cached; + # Fresh hit: still inside the freshness window, so serve without any call. + if (defined $cached && ref($cached) eq 'HASH') { + return $cached->{pr_data} + if defined $cached->{fresh_until} && time() < $cached->{fresh_until}; + } + + # Stale hit with etags: revalidate with conditional requests. 304 responses + # are free (GitHub does not count them against the rate limit), so an + # unchanged PR costs nothing beyond the round trip. + if ( defined $cached + && ref($cached) eq 'HASH' + && ref($cached->{pr_data}) eq 'HASH' + && ($cached->{pr_etag} || $cached->{reviews_etag})) + { + return _revalidate_pull_request($ua, $cache_key, $base, $api_url, $cached); + } + + # Miss (or an entry without etags, e.g. a cached inaccessible result): do a + # full fetch. my $pr_response = _github_get($ua, $api_url); unless ($pr_response->{ok}) { - WARN("GitHub: failed to fetch PR $url: " . $pr_response->{errmsg}); + _warn_fetch_failure($url, $pr_response); return _cache_inaccessible($cache_key, $base); } @@ -135,6 +157,95 @@ sub _fetch_pull_request { return _cache_inaccessible($cache_key, $base); } + my $reviews_response = _github_get($ua, $reviews_url); + my @reviews; + if ($reviews_response->{ok}) { + @reviews = _summarize_reviews($reviews_response->{data}); + } + else { + _warn_fetch_failure("$url reviews", $reviews_response); + } + + my $pr_data = {%$base, _pr_summary_fields($pr), reviews => \@reviews}; + + _store_wrapper($cache_key, { + pr_data => $pr_data, + pr_etag => $pr_response->{etag}, + reviews_etag => $reviews_response->{ok} ? $reviews_response->{etag} : undef, + }); + + return $pr_data; +} + +# Revalidate a stale-but-cached PR using conditional requests. On a 304 we +# reuse the cached fields; on a 200 we recompute from the fresh body. An error +# on the PR endpoint is fatal (falls back to inaccessible); a reviews error is +# non-fatal and yields empty reviews, matching the full-fetch path. +sub _revalidate_pull_request { + my ($ua, $cache_key, $base, $api_url, $cached) = @_; + + my $reviews_url = $api_url . '/reviews?per_page=' . GITHUB_REVIEWS_PER_PAGE; + my $old_data = $cached->{pr_data}; + + # PR endpoint. + my $pr_response = _github_get($ua, $api_url, $cached->{pr_etag}); + unless ($pr_response->{ok}) { + _warn_fetch_failure($old_data->{url}, $pr_response); + return _cache_inaccessible($cache_key, $base); + } + + my ($pr_fields, $pr_etag); + if ($pr_response->{not_modified}) { + + # Unchanged: reuse cached base fields and keep the existing etag. + $pr_fields = { + title => $old_data->{title}, + state => $old_data->{state}, + author => $old_data->{author}, + labels => $old_data->{labels}, + inaccessible => 0, + }; + $pr_etag = $cached->{pr_etag}; + } + else { + my $pr = $pr_response->{data}; + unless (ref($pr) eq 'HASH') { + WARN("GitHub: unexpected response shape for PR " . $old_data->{url}); + return _cache_inaccessible($cache_key, $base); + } + $pr_fields = {_pr_summary_fields($pr)}; + $pr_etag = $pr_response->{etag}; + } + + # Reviews endpoint. + my $reviews_response = _github_get($ua, $reviews_url, $cached->{reviews_etag}); + my ($reviews, $reviews_etag); + if (!$reviews_response->{ok}) { + _warn_fetch_failure($old_data->{url} . ' reviews', $reviews_response); + $reviews = []; + $reviews_etag = undef; + } + elsif ($reviews_response->{not_modified}) { + $reviews = $old_data->{reviews} // []; + $reviews_etag = $cached->{reviews_etag}; + } + else { + $reviews = [_summarize_reviews($reviews_response->{data})]; + $reviews_etag = $reviews_response->{etag}; + } + + my $pr_data = {%$base, %$pr_fields, reviews => $reviews}; + + _store_wrapper($cache_key, + {pr_data => $pr_data, pr_etag => $pr_etag, reviews_etag => $reviews_etag}); + + return $pr_data; +} + +# Derive the servable summary fields (title/state/author/labels) from a PR body. +sub _pr_summary_fields { + my ($pr) = @_; + my $state; if ($pr->{draft}) { $state = 'draft'; @@ -149,38 +260,59 @@ sub _fetch_pull_request { $state = 'open'; } - my @labels = map { $_->{name} } @{$pr->{labels} // []}; - - my $reviews_response - = _github_get($ua, $api_url . '/reviews?per_page=' . GITHUB_REVIEWS_PER_PAGE); - my @reviews; - if ($reviews_response->{ok}) { - @reviews = _summarize_reviews($reviews_response->{data}); - } - - my $pr_data = { - %$base, + return ( title => $pr->{title}, state => $state, author => ref($pr->{user}) eq 'HASH' ? $pr->{user}{login} : undef, - reviews => \@reviews, - labels => \@labels, + labels => [map { $_->{name} } @{$pr->{labels} // []}], inaccessible => 0, - }; + ); +} - Bugzilla->memcached->set_data( - {key => $cache_key, value => $pr_data, expires_in => GITHUB_CACHE_SECONDS}); +# Store the versioned cache wrapper. The freshness window (GITHUB_CACHE_SECONDS) +# is tracked in-band via fresh_until, while the hard memcached TTL is the much +# longer GITHUB_REVALIDATE_SECONDS so the etags outlive the freshness window and +# remain available for conditional revalidation. +sub _store_wrapper { + my ($cache_key, $wrapper) = @_; - return $pr_data; + $wrapper->{fresh_until} = time() + GITHUB_CACHE_SECONDS; + Bugzilla->memcached->set_data({ + key => $cache_key, + value => $wrapper, + expires_in => GITHUB_REVALIDATE_SECONDS, + }); +} + +# Classify a fetch failure and log it distinctly so a globally-misconfigured +# token (401/403 across many repos) is unmistakable and doesn't look like an +# ordinary private/deleted PR (404). +sub _warn_fetch_failure { + my ($what, $response) = @_; + + my $status = $response->{status} // 0; + if ($status == 401 || $status == 403) { + WARN("GitHub: auth/permission failure ($status) for $what" + . " - check github_api_token scope/validity"); + } + elsif ($status == 404) { + WARN("GitHub: PR not found or private ($status): $what"); + } + else { + WARN("GitHub: failed to fetch $what: " . $response->{errmsg}); + } } sub _cache_inaccessible { my ($cache_key, $base) = @_; my $error_data = {%$base, inaccessible => 1}; + + # Inaccessible entries carry no etags and use the shorter error TTL so we + # recover quickly once the PR becomes reachable again. Bugzilla->memcached->set_data({ key => $cache_key, - value => $error_data, + value => {pr_data => $error_data, fresh_until => time() + GITHUB_ERROR_CACHE_SECONDS}, expires_in => GITHUB_ERROR_CACHE_SECONDS, }); @@ -188,24 +320,44 @@ sub _cache_inaccessible { } sub _github_get { - my ($ua, $url) = @_; + my ($ua, $url, $etag) = @_; - my $response = $ua->get( - $url, + my @headers = ( 'Accept' => 'application/vnd.github+json', 'X-GitHub-Api-Version' => '2022-11-28', ); + push @headers, ('If-None-Match' => $etag) if defined $etag; + + my $response = $ua->get($url, @headers); + + # LWP treats 304 as non-success, but for a conditional request it means the + # cached copy is still valid. GitHub echoes the ETag on a 304, so carry it + # through to keep the stored value current. + if ($response->code == 304) { + return { + ok => 1, + not_modified => 1, + status => 304, + etag => $response->header('ETag'), + }; + } unless ($response->is_success) { - return {ok => 0, errmsg => $response->status_line}; + return {ok => 0, status => $response->code, errmsg => $response->status_line}; } my $data = eval { decode_json($response->decoded_content) }; if ($@) { - return {ok => 0, errmsg => "JSON parse error: $@"}; + return {ok => 0, status => $response->code, errmsg => "JSON parse error: $@"}; } - return {ok => 1, data => $data}; + return { + ok => 1, + not_modified => 0, + status => $response->code, + etag => $response->header('ETag'), + data => $data, + }; } sub _summarize_reviews { diff --git a/extensions/GitHubPullRequests/lib/Constants.pm b/extensions/GitHubPullRequests/lib/Constants.pm index 50346bb0e6..3e564e3905 100644 --- a/extensions/GitHubPullRequests/lib/Constants.pm +++ b/extensions/GitHubPullRequests/lib/Constants.pm @@ -19,6 +19,7 @@ our @EXPORT = qw( GITHUB_API_BASE GITHUB_API_TIMEOUT GITHUB_CACHE_SECONDS + GITHUB_REVALIDATE_SECONDS GITHUB_ERROR_CACHE_SECONDS GITHUB_MAX_PULL_REQUESTS GITHUB_REVIEWS_PER_PAGE @@ -40,10 +41,18 @@ use constant GITHUB_PR_REGEX => use constant GITHUB_API_BASE => 'https://api.github.com'; use constant GITHUB_API_TIMEOUT => 10; -# How long (in seconds) to cache a PR's summary in memcached. GitHub's -# unauthenticated rate limit is low (60 req/hr per IP) and authenticated is -# 5000/hr, so caching avoids re-fetching the same PR on every bug view. -use constant GITHUB_CACHE_SECONDS => 300; +# How long (in seconds) a cached PR summary is served without contacting GitHub +# (the freshness window). Once this elapses we revalidate with conditional +# requests rather than doing a full re-fetch; GitHub does not count 304 Not +# Modified responses against the rate limit, so revalidation is effectively +# free and a longer freshness window is safe. +use constant GITHUB_CACHE_SECONDS => 900; + +# Hard memcached TTL for a cached PR summary. This is deliberately much longer +# than the freshness window so the stored ETags survive past GITHUB_CACHE_SECONDS +# and remain available for conditional (If-None-Match) revalidation. Without +# this the ETag would expire exactly when we want to use it. +use constant GITHUB_REVALIDATE_SECONDS => 86_400; # Cache inaccessible/failed lookups for a shorter period so that persistent # failures (rate limiting, outages, private repos) don't re-hit GitHub on every diff --git a/qa/t/rest_github_pull_requests.t b/qa/t/rest_github_pull_requests.t index 00d03485a0..9aae8a77ee 100644 --- a/qa/t/rest_github_pull_requests.t +++ b/qa/t/rest_github_pull_requests.t @@ -102,10 +102,19 @@ my %mock_prs = ( }, ); +# The cache entry is now a versioned wrapper (github_pr.v2.): the summary +# we serve lives under pr_data, alongside revalidation metadata (etags and a +# fresh_until epoch). A fresh_until in the future makes this a "fresh hit", so +# the endpoint serves pr_data verbatim without any outbound request to GitHub. foreach my $pr_url (keys %mock_prs) { Bugzilla->memcached->set_data({ - key => "github_pr.$pr_url", - value => $mock_prs{$pr_url}, + key => "github_pr.v2.$pr_url", + value => { + pr_data => $mock_prs{$pr_url}, + pr_etag => undef, + reviews_etag => undef, + fresh_until => time() + 300, + }, expires_in => 300, }); }