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

Clean up stale merge request HEAD ref

See merge request gitlab-org/gitlab!41555

app/services/concerns/merge_requests/removes_refs.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  module RemovesRefs
+    def cleanup_refs(merge_request)
+      CleanupRefsService.schedule(merge_request)
+    end
+  end
+end

app/services/merge_requests/cleanup_refs_service.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  class CleanupRefsService
+    include BaseServiceUtility
+
+    TIME_THRESHOLD = 14.days
+
+    attr_reader :merge_request
+
+    def self.schedule(merge_request)
+      MergeRequestCleanupRefsWorker.perform_in(TIME_THRESHOLD, merge_request.id)
+    end
+
+    def initialize(merge_request)
+      @merge_request = merge_request
+      @repository = merge_request.project.repository
+      @ref_path = merge_request.ref_path
+      @ref_head_sha = @repository.commit(merge_request.ref_path).id
+    end
+
+    def execute
+      return error("Merge request has not been closed nor merged for #{TIME_THRESHOLD.inspect}.") unless eligible?
+
+      # Ensure that commit shas of refs are kept around so we won't lose them when GC runs.
+      keep_around
+
+      return error('Failed to create keep around refs.') unless kept_around?
+
+      delete_refs
+      success
+    end
+
+    private
+
+    attr_reader :repository, :ref_path, :ref_head_sha
+
+    def eligible?
+      return met_time_threshold?(merge_request.metrics&.latest_closed_at) if merge_request.closed?
+
+      merge_request.merged? && met_time_threshold?(merge_request.metrics&.merged_at)
+    end
+
+    def met_time_threshold?(attr)
+      attr.nil? || attr.to_i <= TIME_THRESHOLD.ago.to_i
+    end
+
+    def kept_around?
+      Gitlab::Git::KeepAround.new(repository).kept_around?(ref_head_sha)
+    end
+
+    def keep_around
+      repository.keep_around(ref_head_sha)
+    end
+
+    def delete_refs
+      repository.delete_refs(ref_path)
+    end
+  end
+end

app/services/merge_requests/close_service.rb

@@ -2,6 +2,8 @@
 
 module MergeRequests
   class CloseService < MergeRequests::BaseService
+    include RemovesRefs
+
     def execute(merge_request, commit = nil)
       return merge_request unless can?(current_user, :update_merge_request, merge_request)
 
@@ -19,6 +21,7 @@ module MergeRequests
         merge_request.update_project_counter_caches
         cleanup_environments(merge_request)
         abort_auto_merge(merge_request, 'merge request was closed')
+        cleanup_refs(merge_request)
       end
 
       merge_request

app/services/merge_requests/post_merge_service.rb

@@ -7,6 +7,8 @@ module MergeRequests
   # and execute all hooks and notifications
   #
   class PostMergeService < MergeRequests::BaseService
+    include RemovesRefs
+
     def execute(merge_request)
       merge_request.mark_as_merged
       close_issues(merge_request)
@@ -20,6 +22,7 @@ module MergeRequests
       delete_non_latest_diffs(merge_request)
       cancel_review_app_jobs!(merge_request)
       cleanup_environments(merge_request)
+      cleanup_refs(merge_request)
     end
 
     private

app/views/projects/merge_requests/_how_to_merge.html.haml

@@ -53,4 +53,4 @@
           %strong Tip:
           = succeed '.' do
             You can also checkout merge requests locally by
-            = link_to 'following these guidelines', help_page_path('user/project/merge_requests/reviewing_and_managing_merge_requests.md', anchor: "checkout-merge-requests-locally"), target: '_blank', rel: 'noopener noreferrer'
+            = link_to 'following these guidelines', help_page_path('user/project/merge_requests/reviewing_and_managing_merge_requests.md', anchor: "checkout-merge-requests-locally-through-the-head-ref"), target: '_blank', rel: 'noopener noreferrer'

app/workers/all_queues.yml

@@ -1492,6 +1492,14 @@
   :weight: 5
   :idempotent:
   :tags: []
+- :name: merge_request_cleanup_refs
+  :feature_category: :source_code_management
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: merge_request_mergeability_check
   :feature_category: :source_code_management
   :has_external_dependencies:

app/workers/merge_request_cleanup_refs_worker.rb

+# frozen_string_literal: true
+
+class MergeRequestCleanupRefsWorker
+  include ApplicationWorker
+
+  feature_category :source_code_management
+  idempotent!
+
+  def perform(merge_request_id)
+    merge_request = MergeRequest.find_by_id(merge_request_id)
+
+    unless merge_request
+      logger.error("Failed to find merge request with ID: #{merge_request_id}")
+      return
+    end
+
+    result = ::MergeRequests::CleanupRefsService.new(merge_request).execute
+
+    return if result[:status] == :success
+
+    logger.error("Failed cleanup refs of merge request (#{merge_request_id}): #{result[:message]}")
+  end
+end

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

+---
+title: Clean up stale merge request HEAD ref
+merge_request: 41555
+author:
+type: performance

config/sidekiq_queues.yml

@@ -154,6 +154,8 @@
   - 2
 - - merge
   - 5
+- - merge_request_cleanup_refs
+  - 1
 - - merge_request_mergeability_check
   - 1
 - - metrics_dashboard_prune_old_annotations

doc/README.md

@@ -198,7 +198,7 @@ The following documentation relates to the DevOps **Create** stage:
 
 | Create topics - Merge Requests                                                                                                              | Description                                                                                                                           |
 |:--------------------------------------------------------------------------------------------------------------------------------------------|:--------------------------------------------------------------------------------------------------------------------------------------|
-| [Checking out merge requests locally](user/project/merge_requests/reviewing_and_managing_merge_requests.md#checkout-merge-requests-locally) | Tips for working with merge requests locally.                                                                                         |
+| [Checking out merge requests locally](user/project/merge_requests/reviewing_and_managing_merge_requests.md#checkout-merge-requests-locally-through-the-head-ref) | Tips for working with merge requests locally.                                                                                         |
 | [Cherry-picking](user/project/merge_requests/cherry_pick_changes.md)                                                                        | Use GitLab for cherry-picking changes.                                                                                                |
 | [Merge request thread resolution](user/discussions/index.md#moving-a-single-thread-to-a-new-issue)                                          | Resolve threads, move threads in a merge request to an issue, and only allow merge requests to be merged if all threads are resolved. |
 | [Merge requests](user/project/merge_requests/index.md)                                                                                      | Merge request management.                                                                                                             |

doc/user/project/merge_requests/reviewing_and_managing_merge_requests.md

@@ -284,15 +284,26 @@ the command line.
 NOTE: **Note:**
 This section might move in its own document in the future.
 
-### Checkout merge requests locally
+### Checkout merge requests locally through the `head` ref
 
 A merge request contains all the history from a repository, plus the additional
 commits added to the branch associated with the merge request. Here's a few
-tricks to checkout a merge request locally.
+ways to checkout a merge request locally.
 
 Please note that you can checkout a merge request locally even if the source
 project is a fork (even a private fork) of the target project.
 
+This relies on the merge request `head` ref (`refs/merge-requests/:iid/head`)
+that is available for each merge request. It allows checking out a merge
+request via its ID instead of its branch.
+
+[Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/223156) in GitLab
+13.4, 14 days after a merge request gets closed or merged, the merge request
+`head` ref will be deleted. This means that the merge request will not be available
+for local checkout via the merge request `head` ref anymore. The merge request
+can still be re-opened. Also, as long as the merge request's branch
+exists, you can still check out the branch as it won't be affected.
+
 #### Checkout locally by adding a Git alias
 
 Add the following alias to your `~/.gitconfig`:

spec/services/merge_requests/cleanup_refs_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::CleanupRefsService do
+  describe '.schedule' do
+    let(:merge_request) { build(:merge_request) }
+
+    it 'schedules MergeRequestCleanupRefsWorker' do
+      expect(MergeRequestCleanupRefsWorker)
+        .to receive(:perform_in)
+        .with(described_class::TIME_THRESHOLD, merge_request.id)
+
+      described_class.schedule(merge_request)
+    end
+  end
+
+  describe '#execute' do
+    before do
+      # Need to re-enable this as it's being stubbed in spec_helper for
+      # performance reasons but is needed to run for this test.
+      allow(Gitlab::Git::KeepAround).to receive(:execute).and_call_original
+    end
+
+    subject(:result) { described_class.new(merge_request).execute }
+
+    shared_examples_for 'service that cleans up merge request refs' do
+      it 'creates keep around ref and deletes merge request refs' do
+        old_ref_head = ref_head
+
+        aggregate_failures do
+          expect(result[:status]).to eq(:success)
+          expect(kept_around?(old_ref_head)).to be_truthy
+          expect(ref_head).to be_nil
+        end
+      end
+
+      context 'when keep around ref cannot be created' do
+        before do
+          allow_next_instance_of(Gitlab::Git::KeepAround) do |keep_around|
+            expect(keep_around).to receive(:kept_around?).and_return(false)
+          end
+        end
+
+        it_behaves_like 'service that does not clean up merge request refs'
+      end
+    end
+
+    shared_examples_for 'service that does not clean up merge request refs' do
+      it 'does not delete merge request refs' do
+        aggregate_failures do
+          expect(result[:status]).to eq(:error)
+          expect(ref_head).to be_present
+        end
+      end
+    end
+
+    context 'when merge request is closed' do
+      let(:merge_request) { create(:merge_request, :closed) }
+
+      context "when closed #{described_class::TIME_THRESHOLD.inspect} ago" do
+        before do
+          merge_request.metrics.update!(latest_closed_at: described_class::TIME_THRESHOLD.ago)
+        end
+
+        it_behaves_like 'service that cleans up merge request refs'
+      end
+
+      context "when closed later than #{described_class::TIME_THRESHOLD.inspect} ago" do
+        before do
+          merge_request.metrics.update!(latest_closed_at: (described_class::TIME_THRESHOLD - 1.day).ago)
+        end
+
+        it_behaves_like 'service that does not clean up merge request refs'
+      end
+    end
+
+    context 'when merge request is merged' do
+      let(:merge_request) { create(:merge_request, :merged) }
+
+      context "when merged #{described_class::TIME_THRESHOLD.inspect} ago" do
+        before do
+          merge_request.metrics.update!(merged_at: described_class::TIME_THRESHOLD.ago)
+        end
+
+        it_behaves_like 'service that cleans up merge request refs'
+      end
+
+      context "when merged later than #{described_class::TIME_THRESHOLD.inspect} ago" do
+        before do
+          merge_request.metrics.update!(merged_at: (described_class::TIME_THRESHOLD - 1.day).ago)
+        end
+
+        it_behaves_like 'service that does not clean up merge request refs'
+      end
+    end
+
+    context 'when merge request is not closed nor merged' do
+      let(:merge_request) { create(:merge_request, :opened) }
+
+      it_behaves_like 'service that does not clean up merge request refs'
+    end
+  end
+
+  def kept_around?(commit)
+    Gitlab::Git::KeepAround.new(merge_request.project.repository).kept_around?(commit.id)
+  end
+
+  def ref_head
+    merge_request.project.repository.commit(merge_request.ref_path)
+  end
+end

spec/services/merge_requests/close_service_spec.rb

@@ -99,6 +99,12 @@ RSpec.describe MergeRequests::CloseService do
       described_class.new(project, user).execute(merge_request)
     end
 
+    it 'schedules CleanupRefsService' do
+      expect(MergeRequests::CleanupRefsService).to receive(:schedule).with(merge_request)
+
+      described_class.new(project, user).execute(merge_request)
+    end
+
     context 'current user is not authorized to close merge request' do
       before do
         perform_enqueued_jobs do

spec/services/merge_requests/post_merge_service_spec.rb

@@ -72,6 +72,12 @@ RSpec.describe MergeRequests::PostMergeService do
       subject
     end
 
+    it 'schedules CleanupRefsService' do
+      expect(MergeRequests::CleanupRefsService).to receive(:schedule).with(merge_request)
+
+      subject
+    end
+
     context 'when the merge request has review apps' do
       it 'cancels all review app deployments' do
         pipeline = create(:ci_pipeline,

spec/workers/merge_request_cleanup_refs_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequestCleanupRefsWorker do
+  describe '#perform' do
+    context 'when merge request exists' do
+      let(:merge_request) { create(:merge_request) }
+      let(:job_args) { merge_request.id }
+
+      include_examples 'an idempotent worker' do
+        it 'calls MergeRequests::CleanupRefsService#execute' do
+          expect_next_instance_of(MergeRequests::CleanupRefsService, merge_request) do |svc|
+            expect(svc).to receive(:execute).and_call_original
+          end.twice
+
+          subject
+        end
+      end
+    end
+
+    context 'when merge request does not exist' do
+      it 'does not call MergeRequests::CleanupRefsService' do
+        expect(MergeRequests::CleanupRefsService).not_to receive(:new)
+
+        perform_multiple(1)
+      end
+    end
+  end
+end