Avoid caching comment diff fragments if an error is shown.

Review Request #12295 — Created May 22, 2022 and submitted

Information

Review Board
release-4.0.x

Reviewers

We attempt to aggressively cache any kind of rendered diff content, both
server-side and client-side, in order to avoid extra repository lookups,
diff application, and rendering.

The one time we don't want to do this is when there's an error being
shown. Errors can be temporary, and should never be cached. We avoid
caching these server-side, but for diff fragments, we were still
allowing the browser to cache these.

This was happening due to our ETag logic, which didn't know anything
about the comment content or errors (for performance reasons), and
happily assigned an ETag if there was an issue. Partly, that's because
we'd return an HTTP 200 for results, even if there are errors, as the
result of this view is a payload containing results, so we don't want to
necessarily fail the whole view if one thing within fails.

This ultimately meant that a user who encountered a temporary error
would continue to see that error until they cleared cache or checked in
another browser.

This change avoids client-side caching of errors. If there's an error,
we set the cache control headers to disallow caching and to expire the
age, forcing a re-request on the next attempt.

Unit tests passed.

Simulated this by setting a GitHub access token to a garbage value and
loading a review with comments. Saw the errors. In a shell, I then set
the token back and did a reload of the page. Saw that it re-fetched and
got the correct content, instead of stale cached content.

Without this patch, that repro case resulted in seeing the stale cached
content after a reload.

Summary ID
Avoid caching comment diff fragments if an error is shown.
We attempt to aggressively cache any kind of rendered diff content, both server-side and client-side, in order to avoid extra repository lookups, diff application, and rendering. The one time we don't want to do this is when there's an error being shown. Errors can be temporary, and should never be cached. We avoid caching these server-side, but for diff fragments, we were still allowing the browser to cache these. This was happening due to our ETag logic, which didn't know anything about the comment content or errors (for performance reasons), and happily assigned an ETag if there was an issue. Partly, that's because we'd return an HTTP 200 for results, even if there are errors, as the result of this view is a payload containing results, so we don't want to necessarily fail the whole view if one thing within fails. This ultimately meant that a user who encountered a temporary error would continue to see that error until they cleared cache or checked in another browser. This change avoids client-side caching of errors. If there's an error, we set the cache control headers to disallow caching and to expire the age, forcing a re-request on the next attempt.
f8ec8f2de4e18b70ed461498a8771d5379e557c7
david
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (5bbcc12)
Loading...