Remove stale merge refs

14 days after a merge request gets closed/merged, its merge ref
will be deleted and moved as a keep-around ref (if not yet added).

This is to ensure that the commit won't be deleted when GC runs
and still being able to reduce the number of advertised refs on
git push/pull.

Before deletion, it'll be cached to `merge_ref_sha` column of
`merge_requests` table so we can still display the merge ref diff.

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