Use GitLab version number (or similar) for merge request diffs caching
In https://gitlab.com/gitlab-org/gitlab-ce/issues/48801 and https://gitlab.com/gitlab-org/gitlab-ce/issues/50793 we saw bugs due to MR diff caching. Both of these bugs were because the data stored in the cache changed, but the key didn't.
This comes from:
-
Gitlab::Diff::FileCollection::MergeRequestDiffis a wrapper class that allows caching highlighted MR diffs in Redis, to avoid having to fetch blobs each time. - It uses
Gitlab::Diff::Line::SERIALIZE_KEYSas part of its cache key, along with info about the MR diff itself: https://gitlab.com/gitlab-org/gitlab-ce/blob/v11.2.3/lib/gitlab/diff/file_collection/merge_request_diff.rb#L37 -
Gitlab::Diff::Line::SERIALIZE_KEYSmay not necessarily change, even if the semantics of the cached values change (https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2463).
In the CacheableAttributes concern, we use this as the cache key: "#{name}:#{Gitlab::VERSION}:#{Rails.version}"
I propose that we use the GitLab version in the cache key for MR diffs, instead of Gitlab::Diff::Line::SERIALIZE_KEYS.
In favour:
- Much simpler to reason about. Every time the GitLab version changes, the cache key changes.
- Similarly, it's much harder to get wrong. It's not possible to miss this in an update, and right now the situation is that it is far too easy to miss this.
- It's unreasonably hard to write a test that verifies the current behaviour across upgrades.
In opposition:
- This has only changed twice. Both were recently, and both were bad issues that were hard to solve, but it's still only two data points.
- We cache this for a week right now. We might bump GitLab versions on GitLab.com more often than that, keeping stale data around in the cache.
- To mitigate, we could reduce the TTL further.
- As a result of 2, we'd probably see more frequent cache misses, leading to an increase in response times.
I have only thought of this since yesterday, so there might be other points I'm missing. But writing this now, I think the points in favour are much stronger than the points against.
Edited by Sean McGivern