Delete non-latest merge request diff files upon diffs reload

9 files changed, 117 insertions, 30 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 3df1130a6e2..f112c06e26f 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -378,6 +378,10 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
+  def non_latest_diffs
+    merge_request_diffs.where.not(id: merge_request_diff.id)
+  end
+
   def diff_size
     # Calling `merge_request_diff.diffs.real_size` will also perform
     # highlighting, which we don't need here.
@@ -619,18 +623,7 @@ class MergeRequest < ActiveRecord::Base
   def reload_diff(current_user = nil)
     return unless open?
 
-    old_diff_refs = self.diff_refs
-    new_diff = create_merge_request_diff
-
-    MergeRequests::MergeRequestDiffCacheService.new.execute(self, new_diff)
-
-    new_diff_refs = self.diff_refs
-
-    update_diff_discussion_positions(
-      old_diff_refs: old_diff_refs,
-      new_diff_refs: new_diff_refs,
-      current_user: current_user
-    )
+    MergeRequests::ReloadDiffsService.new(self, current_user).execute
   end
 
   def check_if_can_be_merged
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 06aa67c600f..3d72c447b4b 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -3,6 +3,7 @@ class MergeRequestDiff < ActiveRecord::Base
   include Importable
   include ManualInverseAssociation
   include IgnorableColumn
+  include EachBatch
 
   # Don't display more than 100 commits at once
   COMMITS_SAFE_SIZE = 100
@@ -17,8 +18,14 @@ class MergeRequestDiff < ActiveRecord::Base
   has_many :merge_request_diff_commits, -> { order(:merge_request_diff_id, :relative_order) }
 
   state_machine :state, initial: :empty do
+    event :clean do
+      transition any => :without_files
+    end
+
     state :collected
     state :overflow
+    # Diff files have been deleted by the system
+    state :without_files
     # Deprecated states: these are no longer used but these values may still occur
     # in the database.
     state :timeout
@@ -27,6 +34,7 @@ class MergeRequestDiff < ActiveRecord::Base
     state :overflow_diff_lines_limit
   end
 
+  scope :with_files, -> { without_states(:without_files, :empty) }
   scope :viewable, -> { without_state(:empty) }
   scope :by_commit_sha, ->(sha) do
     joins(:merge_request_diff_commits).where(merge_request_diff_commits: { sha: sha }).reorder(nil)
@@ -42,6 +50,10 @@ class MergeRequestDiff < ActiveRecord::Base
     find_by(start_commit_sha: diff_refs.start_sha, head_commit_sha: diff_refs.head_sha, base_commit_sha: diff_refs.base_sha)
   end
 
+  def viewable?
+    collected? || without_files? || overflow?
+  end
+
   # Collect information about commits and diff from repository
   # and save it to the database as serialized data
   def save_git_content
@@ -170,6 +182,21 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def diffs(diff_options = nil)
+    if without_files? && comparison = diff_refs.compare_in(project)
+      # It should fetch the repository when diffs are cleaned by the system.
+      # We don't keep these for storage overload purposes.
+      # See https://gitlab.com/gitlab-org/gitlab-ce/issues/37639
+      comparison.diffs(diff_options)
+    else
+      diffs_collection(diff_options)
+    end
+  end
+
+  # Should always return the DB persisted diffs collection
+  # (e.g. Gitlab::Diff::FileCollection::MergeRequestDiff.
+  # It's useful when trying to invalidate old caches through
+  # FileCollection::MergeRequestDiff#clear_cache!
+  def diffs_collection(diff_options = nil)
     Gitlab::Diff::FileCollection::MergeRequestDiff.new(self, diff_options: diff_options)
   end
 
diff --git a/app/services/merge_requests/delete_non_latest_diffs_service.rb b/app/services/merge_requests/delete_non_latest_diffs_service.rb
new file mode 100644
index 00000000000..40079b21189
--- /dev/null
+++ b/app/services/merge_requests/delete_non_latest_diffs_service.rb
@@ -0,0 +1,18 @@
+module MergeRequests
+  class DeleteNonLatestDiffsService
+    BATCH_SIZE = 10
+
+    def initialize(merge_request)
+      @merge_request = merge_request
+    end
+
+    def execute
+      diffs = @merge_request.non_latest_diffs.with_files
+
+      diffs.each_batch(of: BATCH_SIZE) do |relation, index|
+        ids = relation.pluck(:id).map { |id| [id] }
+        DeleteDiffFilesWorker.bulk_perform_in(index * 5.minutes, ids)
+      end
+    end
+  end
+end
diff --git a/app/services/merge_requests/merge_request_diff_cache_service.rb b/app/services/merge_requests/merge_request_diff_cache_service.rb
deleted file mode 100644
index 10aa9ae609c..00000000000
--- a/app/services/merge_requests/merge_request_diff_cache_service.rb
+++ /dev/null
@@ -1,17 +0,0 @@
-module MergeRequests
-  class MergeRequestDiffCacheService
-    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/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb
index c78e78afcd1..5b160ffba67 100644
--- a/app/services/merge_requests/post_merge_service.rb
+++ b/app/services/merge_requests/post_merge_service.rb
@@ -15,6 +15,7 @@ module MergeRequests
       execute_hooks(merge_request, 'merge')
       invalidate_cache_counts(merge_request, users: merge_request.assignees)
       merge_request.update_project_counter_caches
+      delete_non_latest_diffs(merge_request)
     end
 
     private
@@ -31,6 +32,10 @@ module MergeRequests
       end
     end
 
+    def delete_non_latest_diffs(merge_request)
+      DeleteNonLatestDiffsService.new(merge_request).execute
+    end
+
     def create_merge_event(merge_request, current_user)
       EventCreateService.new.merge_mr(merge_request, current_user)
     end
diff --git a/app/services/merge_requests/reload_diffs_service.rb b/app/services/merge_requests/reload_diffs_service.rb
new file mode 100644
index 00000000000..2ec7b403903
--- /dev/null
+++ b/app/services/merge_requests/reload_diffs_service.rb
@@ -0,0 +1,43 @@
+module MergeRequests
+  class ReloadDiffsService
+    def initialize(merge_request, current_user)
+      @merge_request = merge_request
+      @current_user = current_user
+    end
+
+    def execute
+      old_diff_refs = merge_request.diff_refs
+      new_diff = merge_request.create_merge_request_diff
+
+      clear_cache(new_diff)
+      update_diff_discussion_positions(old_diff_refs)
+    end
+
+    private
+
+    attr_reader :merge_request, :current_user
+
+    def update_diff_discussion_positions(old_diff_refs)
+      new_diff_refs = merge_request.diff_refs
+
+      merge_request.update_diff_discussion_positions(old_diff_refs: old_diff_refs,
+                                                     new_diff_refs: new_diff_refs,
+                                                     current_user: current_user)
+    end
+
+    def clear_cache(new_diff)
+      # Executing the iteration we cache highlighted diffs for each diff file of
+      # MergeRequestDiff.
+      new_diff.diffs_collection.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_collection.clear_cache!
+      end
+    end
+  end
+end
diff --git a/app/views/projects/merge_requests/diffs/_diffs.html.haml b/app/views/projects/merge_requests/diffs/_diffs.html.haml
index 19659fe5140..bf3df0abf86 100644
--- a/app/views/projects/merge_requests/diffs/_diffs.html.haml
+++ b/app/views/projects/merge_requests/diffs/_diffs.html.haml
@@ -16,6 +16,6 @@
           %span.ref-name= @merge_request.target_branch
           .text-center= link_to 'Create commit', project_new_blob_path(@project, @merge_request.source_branch), class: 'btn btn-save'
 - else
-  - diff_viewable = @merge_request_diff ? @merge_request_diff.collected? || @merge_request_diff.overflow? : true
+  - diff_viewable = @merge_request_diff ? @merge_request_diff.viewable? : true
   - if diff_viewable
     = render "projects/diffs/diffs", diffs: @diffs, environment: @environment, merge_request: true
diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index 30b6796a7d6..026f756582d 100644
--- a/app/workers/all_queues.yml
+++ b/app/workers/all_queues.yml
@@ -118,3 +118,4 @@
 - web_hook
 - repository_update_remote_mirror
 - create_note_diff_file
+- delete_diff_files
diff --git a/app/workers/delete_diff_files_worker.rb b/app/workers/delete_diff_files_worker.rb
new file mode 100644
index 00000000000..bb8fbb9c373
--- /dev/null
+++ b/app/workers/delete_diff_files_worker.rb
@@ -0,0 +1,17 @@
+class DeleteDiffFilesWorker
+  include ApplicationWorker
+
+  def perform(merge_request_diff_id)
+    merge_request_diff = MergeRequestDiff.find(merge_request_diff_id)
+
+    return if merge_request_diff.without_files?
+
+    MergeRequestDiff.transaction do
+      merge_request_diff.clean!
+
+      MergeRequestDiffFile
+        .where(merge_request_diff_id: merge_request_diff.id)
+        .delete_all
+    end
+  end
+end