Merge branch '44191-reduce-redis-usage-from-merge-request-diffs-caching' into 'master'

Resolve "Reduce Redis usage from merge request diffs caching"

Closes #44191

See merge request gitlab-org/gitlab-ce!17746

7 files changed, 57 insertions, 18 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index c2bae379a94..149ef7ec429 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -579,9 +579,10 @@ class MergeRequest < ActiveRecord::Base
     return unless open?
 
     old_diff_refs = self.diff_refs
+    new_diff = create_merge_request_diff
+
+    MergeRequests::MergeRequestDiffCacheService.new.execute(self, new_diff)
 
-    create_merge_request_diff
-    MergeRequests::MergeRequestDiffCacheService.new.execute(self)
     new_diff_refs = self.diff_refs
 
     update_diff_discussion_positions(
diff --git a/app/services/merge_requests/merge_request_diff_cache_service.rb b/app/services/merge_requests/merge_request_diff_cache_service.rb
index 2945a7fd4e4..10aa9ae609c 100644
--- a/app/services/merge_requests/merge_request_diff_cache_service.rb
+++ b/app/services/merge_requests/merge_request_diff_cache_service.rb
@@ -1,8 +1,17 @@
 module MergeRequests
   class MergeRequestDiffCacheService
-    def execute(merge_request)
+    def execute(merge_request, new_diff)
       # Executing the iteration we cache all the highlighted diff information
       merge_request.diffs.diff_files.to_a
+
+      # Remove cache for all diffs on this MR. Do not use the association on the
+      # model, as that will interfere with other actions happening when
+      # reloading the diff.
+      MergeRequestDiff.where(merge_request: merge_request).each do |merge_request_diff|
+        next if merge_request_diff == new_diff
+
+        merge_request_diff.diffs.clear_cache!
+      end
     end
   end
 end
diff --git a/changelogs/unreleased/44191-reduce-redis-usage-from-merge-request-diffs-caching.yml b/changelogs/unreleased/44191-reduce-redis-usage-from-merge-request-diffs-caching.yml
new file mode 100644
index 00000000000..8fdca6eec83
--- /dev/null
+++ b/changelogs/unreleased/44191-reduce-redis-usage-from-merge-request-diffs-caching.yml
@@ -0,0 +1,5 @@
+---
+title: Stop caching highlighted diffs in Redis unnecessarily
+merge_request: 17746
+author:
+type: performance
diff --git a/lib/gitlab/diff/file_collection/merge_request_diff.rb b/lib/gitlab/diff/file_collection/merge_request_diff.rb
index ff68bc7303a..c358ae428cf 100644
--- a/lib/gitlab/diff/file_collection/merge_request_diff.rb
+++ b/lib/gitlab/diff/file_collection/merge_request_diff.rb
@@ -29,6 +29,14 @@ module Gitlab
           @merge_request_diff.real_size
         end
 
+        def clear_cache!
+          Rails.cache.delete(cache_key)
+        end
+
+        def cache_key
+          [@merge_request_diff, 'highlighted-diff-files', diff_options]
+        end
+
         private
 
         def highlight_diff_file_from_cache!(diff_file, cache_diff_lines)
@@ -64,16 +72,12 @@ module Gitlab
         end
 
         def store_highlight_cache
-          Rails.cache.write(cache_key, highlight_cache) if @highlight_cache_was_empty
+          Rails.cache.write(cache_key, highlight_cache, expires_in: 1.week) if @highlight_cache_was_empty
         end
 
         def cacheable?(diff_file)
           @merge_request_diff.present? && diff_file.text? && diff_file.diffable?
         end
-
-        def cache_key
-          [@merge_request_diff, 'highlighted-diff-files', diff_options]
-        end
       end
     end
   end
diff --git a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb
index a067c42b75b..f48ee8924e8 100644
--- a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb
+++ b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb
@@ -12,7 +12,7 @@ describe Gitlab::Diff::FileCollection::MergeRequestDiff do
     diff_files
   end
 
-  it 'does not files marked as undiffable in .gitattributes' do
+  it 'does not highlight files marked as undiffable in .gitattributes' do
     allow_any_instance_of(Gitlab::Diff::File).to receive(:diffable?).and_return(false)
 
     expect_any_instance_of(Gitlab::Diff::File).not_to receive(:highlighted_diff_lines)
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 7986aa31e16..4e783acbd8b 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1544,7 +1544,7 @@ describe MergeRequest do
     end
 
     it "executes diff cache service" do
-      expect_any_instance_of(MergeRequests::MergeRequestDiffCacheService).to receive(:execute).with(subject)
+      expect_any_instance_of(MergeRequests::MergeRequestDiffCacheService).to receive(:execute).with(subject, an_instance_of(MergeRequestDiff))
 
       subject.reload_diff
     end
diff --git a/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb b/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb
index bb46e1dd9ab..57b6165cfb0 100644
--- a/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb
+++ b/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb
@@ -1,19 +1,39 @@
 require 'spec_helper'
 
-describe MergeRequests::MergeRequestDiffCacheService do
+describe MergeRequests::MergeRequestDiffCacheService, :use_clean_rails_memory_store_caching do
   let(:subject) { described_class.new }
+  let(:merge_request) { create(:merge_request) }
 
   describe '#execute' do
-    it 'retrieves the diff files to cache the highlighted result' do
-      merge_request = create(:merge_request)
-      cache_key = [merge_request.merge_request_diff, 'highlighted-diff-files', Gitlab::Diff::FileCollection::MergeRequestDiff.default_options]
-
-      expect(Rails.cache).to receive(:read).with(cache_key).and_return({})
-      expect(Rails.cache).to receive(:write).with(cache_key, anything)
+    before do
       allow_any_instance_of(Gitlab::Diff::File).to receive(:text?).and_return(true)
       allow_any_instance_of(Gitlab::Diff::File).to receive(:diffable?).and_return(true)
+    end
+
+    it 'retrieves the diff files to cache the highlighted result' do
+      new_diff = merge_request.merge_request_diff
+      cache_key = new_diff.diffs.cache_key
+
+      expect(Rails.cache).to receive(:read).with(cache_key).and_call_original
+      expect(Rails.cache).to receive(:write).with(cache_key, anything, anything).and_call_original
+
+      subject.execute(merge_request, new_diff)
+    end
+
+    it 'clears the cache for older diffs on the merge request' do
+      old_diff = merge_request.merge_request_diff
+      old_cache_key = old_diff.diffs.cache_key
+
+      subject.execute(merge_request, old_diff)
+
+      new_diff = merge_request.create_merge_request_diff
+      new_cache_key = new_diff.diffs.cache_key
+
+      expect(Rails.cache).to receive(:delete).with(old_cache_key).and_call_original
+      expect(Rails.cache).to receive(:read).with(new_cache_key).and_call_original
+      expect(Rails.cache).to receive(:write).with(new_cache_key, anything, anything).and_call_original
 
-      subject.execute(merge_request)
+      subject.execute(merge_request, new_diff)
     end
   end
 end