Avoid caching comment diff fragments if an error is shown.
Review Request #12295 — Created May 22, 2022 and submitted
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
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.