Create/update cleanup schedule on schedule and completion

Instead of scheduling sidekiq jobs and keeping them in redis
for 14 days before they get performed, we set the schedule in the
DB.

This is to prevent huge number of scheduled jobs in cases when
there are a lot of closed/merged MRs.

app/models/merge_request/cleanup_schedule.rb

@@ -2,4 +2,6 @@
 
 class MergeRequest::CleanupSchedule < ApplicationRecord
   belongs_to :merge_request, inverse_of: :cleanup_schedule
+
+  validates :scheduled_at, presence: true
 end

app/services/merge_requests/cleanup_refs_service.rb

@@ -9,7 +9,7 @@ module MergeRequests
     attr_reader :merge_request
 
     def self.schedule(merge_request)
-      MergeRequestCleanupRefsWorker.perform_in(TIME_THRESHOLD, merge_request.id)
+      merge_request.create_cleanup_schedule(scheduled_at: TIME_THRESHOLD.from_now)
     end
 
     def initialize(merge_request)
@@ -22,6 +22,7 @@ module MergeRequests
     end
 
     def execute
+      return error("Merge request is not scheduled to be cleaned up yet.") unless scheduled?
       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.
@@ -31,13 +32,17 @@ module MergeRequests
       return error('Failed to cache merge ref sha.') unless cache_merge_ref_sha
 
       delete_refs
-      success
+      success if update_schedule
     end
 
     private
 
     attr_reader :repository, :ref_path, :merge_ref_path, :ref_head_sha, :merge_ref_sha
 
+    def scheduled?
+      merge_request.cleanup_schedule.present? && merge_request.cleanup_schedule.scheduled_at <= Time.current
+    end
+
     def eligible?
       return met_time_threshold?(merge_request.metrics&.latest_closed_at) if merge_request.closed?
 
@@ -71,5 +76,9 @@ module MergeRequests
     def delete_refs
       repository.delete_refs(ref_path, merge_ref_path)
     end
+
+    def update_schedule
+      merge_request.cleanup_schedule.update(completed_at: Time.current)
+    end
   end
 end

app/services/merge_requests/reopen_service.rb

@@ -15,6 +15,7 @@ module MergeRequests
         invalidate_cache_counts(merge_request, users: merge_request.assignees)
         merge_request.update_project_counter_caches
         merge_request.cache_merge_request_closes_issues!(current_user)
+        merge_request.cleanup_schedule&.destroy
       end
 
       merge_request

spec/models/merge_request/cleanup_schedule_spec.rb

@@ -6,4 +6,8 @@ RSpec.describe MergeRequest::CleanupSchedule do
   describe 'associations' do
     it { is_expected.to belong_to(:merge_request) }
   end
+
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:scheduled_at) }
+  end
 end

spec/services/merge_requests/cleanup_refs_service_spec.rb

@@ -4,14 +4,15 @@ require 'spec_helper'
 
 RSpec.describe MergeRequests::CleanupRefsService do
   describe '.schedule' do
-    let(:merge_request) { build(:merge_request) }
+    let(:merge_request) { create(:merge_request) }
 
-    it 'schedules MergeRequestCleanupRefsWorker' do
-      expect(MergeRequestCleanupRefsWorker)
-        .to receive(:perform_in)
-        .with(described_class::TIME_THRESHOLD, merge_request.id)
+    it 'creates a merge request cleanup schedule' do
+      freeze_time do
+        described_class.schedule(merge_request)
 
-      described_class.schedule(merge_request)
+        expect(merge_request.reload.cleanup_schedule.scheduled_at)
+          .to eq(described_class::TIME_THRESHOLD.from_now)
+      end
     end
   end
 
@@ -20,6 +21,8 @@ RSpec.describe MergeRequests::CleanupRefsService 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
+
+      merge_request.create_cleanup_schedule(scheduled_at: described_class::TIME_THRESHOLD.ago)
     end
 
     subject(:result) { described_class.new(merge_request).execute }
@@ -32,6 +35,7 @@ RSpec.describe MergeRequests::CleanupRefsService do
           expect(result[:status]).to eq(:success)
           expect(kept_around?(old_ref_head)).to be_truthy
           expect(ref_head).to be_nil
+          expect(merge_request.cleanup_schedule.completed_at).to be_present
         end
       end
 
@@ -43,6 +47,7 @@ RSpec.describe MergeRequests::CleanupRefsService do
 
         it 'does not fail' do
           expect(result[:status]).to eq(:success)
+          expect(merge_request.cleanup_schedule.completed_at).to be_present
         end
       end
 
@@ -85,6 +90,14 @@ RSpec.describe MergeRequests::CleanupRefsService do
 
         it_behaves_like 'service that does not clean up merge request refs'
       end
+
+      context 'when merge request is not scheduled to be cleaned up yet' do
+        before do
+          merge_request.cleanup_schedule.update!(scheduled_at: 1.day.from_now)
+        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
@@ -92,6 +105,7 @@ RSpec.describe MergeRequests::CleanupRefsService do
         aggregate_failures do
           expect(result[:status]).to eq(:error)
           expect(ref_head).to be_present
+          expect(merge_request.cleanup_schedule.completed_at).not_to be_present
         end
       end
     end

spec/services/merge_requests/reopen_service_spec.rb

@@ -23,6 +23,7 @@ RSpec.describe MergeRequests::ReopenService do
 
       before do
         allow(service).to receive(:execute_hooks)
+        merge_request.create_cleanup_schedule(scheduled_at: Time.current)
 
         perform_enqueued_jobs do
           service.execute(merge_request)
@@ -43,6 +44,10 @@ RSpec.describe MergeRequests::ReopenService do
         expect(email.subject).to include(merge_request.title)
       end
 
+      it 'destroys cleanup schedule record' do
+        expect(merge_request.reload.cleanup_schedule).to be_nil
+      end
+
       context 'note creation' do
         it 'creates resource state event about merge_request reopen' do
           event = merge_request.resource_state_events.last