Merge branch '223156-remove-stale-merge-refs' into 'master'

Remove stale merge refs

See merge request gitlab-org/gitlab!41572

app/models/merge_request.rb

@@ -25,6 +25,7 @@ class MergeRequest < ApplicationRecord
   extend ::Gitlab::Utils::Override
 
   sha_attribute :squash_commit_sha
+  sha_attribute :merge_ref_sha
 
   self.reactive_cache_key = ->(model) { [model.project.id, model.iid] }
   self.reactive_cache_refresh_interval = 10.minutes
@@ -1258,6 +1259,8 @@ class MergeRequest < ApplicationRecord
   # Returns the current merge-ref HEAD commit.
   #
   def merge_ref_head
+    return project.repository.commit(merge_ref_sha) if merge_ref_sha
+
     project.repository.commit(merge_ref_path)
   end
 

app/services/merge_requests/cleanup_refs_service.rb

@@ -16,7 +16,9 @@ module MergeRequests
       @merge_request = merge_request
       @repository = merge_request.project.repository
       @ref_path = merge_request.ref_path
+      @merge_ref_path = merge_request.merge_ref_path
       @ref_head_sha = @repository.commit(merge_request.ref_path).id
+      @merge_ref_sha = merge_request.merge_ref_head&.id
     end
 
     def execute
@@ -26,6 +28,7 @@ module MergeRequests
       keep_around
 
       return error('Failed to create keep around refs.') unless kept_around?
+      return error('Failed to cache merge ref sha.') unless cache_merge_ref_sha
 
       delete_refs
       success
@@ -33,7 +36,7 @@ module MergeRequests
 
     private
 
-    attr_reader :repository, :ref_path, :ref_head_sha
+    attr_reader :repository, :ref_path, :merge_ref_path, :ref_head_sha, :merge_ref_sha
 
     def eligible?
       return met_time_threshold?(merge_request.metrics&.latest_closed_at) if merge_request.closed?
@@ -46,15 +49,27 @@ module MergeRequests
     end
 
     def kept_around?
-      Gitlab::Git::KeepAround.new(repository).kept_around?(ref_head_sha)
+      service = Gitlab::Git::KeepAround.new(repository)
+
+      [ref_head_sha, merge_ref_sha].compact.all? do |sha|
+        service.kept_around?(sha)
+      end
     end
 
     def keep_around
-      repository.keep_around(ref_head_sha)
+      repository.keep_around(ref_head_sha, merge_ref_sha)
+    end
+
+    def cache_merge_ref_sha
+      return true if merge_ref_sha.nil?
+
+      # Caching the merge ref sha is needed before we delete the merge ref so
+      # we can still show the merge ref diff (via `MergeRequest#merge_ref_head`)
+      merge_request.update_column(:merge_ref_sha, merge_ref_sha)
     end
 
     def delete_refs
-      repository.delete_refs(ref_path)
+      repository.delete_refs(ref_path, merge_ref_path)
     end
   end
 end

changelogs/unreleased/223156-remove-stale-merge-refs.yml

+---
+title: Remove stale merge refs
+merge_request: 41572
+author:
+type: performance

db/migrate/20200904092131_add_merge_ref_sha_to_merge_requests.rb

+# frozen_string_literal: true
+
+class AddMergeRefShaToMergeRequests < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :merge_requests, :merge_ref_sha, :binary
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :merge_requests, :merge_ref_sha
+    end
+  end
+end

db/schema_migrations/20200904092131

+024d4448f6cd9b9fd8f6d1892882de596928d0265e91f79c6a52431c8fb3c08b
\ No newline at end of file

db/structure.sql

@@ -13339,6 +13339,7 @@ CREATE TABLE public.merge_requests (
     rebase_jid character varying,
     squash_commit_sha bytea,
     sprint_id bigint,
+    merge_ref_sha bytea,
     CONSTRAINT check_970d272570 CHECK ((lock_version IS NOT NULL))
 );
 

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -206,6 +206,7 @@ MergeRequest:
 - head_pipeline_id
 - discussion_locked
 - allow_maintainer_to_push
+- merge_ref_sha
 MergeRequestDiff:
 - id
 - state

spec/models/merge_request_spec.rb

@@ -4278,4 +4278,30 @@ RSpec.describe MergeRequest, factory_default: :keep do
       expect(merge_request.allows_reviewers?).to be(true)
     end
   end
+
+  describe '#merge_ref_head' do
+    let(:merge_request) { create(:merge_request) }
+
+    context 'when merge_ref_sha is not present' do
+      let!(:result) do
+        MergeRequests::MergeToRefService
+          .new(merge_request.project, merge_request.author)
+          .execute(merge_request)
+      end
+
+      it 'returns the commit based on merge ref path' do
+        expect(merge_request.merge_ref_head.id).to eq(result[:commit_id])
+      end
+    end
+
+    context 'when merge_ref_sha is present' do
+      before do
+        merge_request.update!(merge_ref_sha: merge_request.project.repository.commit.id)
+      end
+
+      it 'returns the commit based on cached merge_ref_sha' do
+        expect(merge_request.merge_ref_head.id).to eq(merge_request.merge_ref_sha)
+      end
+    end
+  end
 end

spec/services/merge_requests/cleanup_refs_service_spec.rb

@@ -35,6 +35,36 @@ RSpec.describe MergeRequests::CleanupRefsService do
         end
       end
 
+      context 'when merge request has merge ref' do
+        before do
+          MergeRequests::MergeToRefService
+            .new(merge_request.project, merge_request.author)
+            .execute(merge_request)
+        end
+
+        it 'caches merge ref sha and deletes merge ref' do
+          old_merge_ref_head = merge_request.merge_ref_head
+
+          aggregate_failures do
+            expect(result[:status]).to eq(:success)
+            expect(kept_around?(old_merge_ref_head)).to be_truthy
+            expect(merge_request.reload.merge_ref_sha).to eq(old_merge_ref_head.id)
+            expect(ref_exists?(merge_request.merge_ref_path)).to be_falsy
+          end
+        end
+
+        context 'when merge ref sha cannot be cached' do
+          before do
+            allow(merge_request)
+              .to receive(:update_column)
+              .with(:merge_ref_sha, merge_request.merge_ref_head.id)
+              .and_return(false)
+          end
+
+          it_behaves_like 'service that does not clean up merge request refs'
+        end
+      end
+
       context 'when keep around ref cannot be created' do
         before do
           allow_next_instance_of(Gitlab::Git::KeepAround) do |keep_around|
@@ -109,4 +139,8 @@ RSpec.describe MergeRequests::CleanupRefsService do
   def ref_head
     merge_request.project.repository.commit(merge_request.ref_path)
   end
+
+  def ref_exists?(ref)
+    merge_request.project.repository.ref_exists?(ref)
+  end
 end