Merge branch 'id-destroy-unreferenced-lfs-objects-in-batches' into 'master'

Remove unreferenced lfs objects in batches

See merge request gitlab-org/gitlab!56959

app/models/lfs_object.rb

@@ -18,6 +18,8 @@ class LfsObject < ApplicationRecord
 
   mount_file_store_uploader LfsObjectUploader
 
+  BATCH_SIZE = 3000
+
   def self.not_linked_to_project(project)
     where('NOT EXISTS (?)',
           project.lfs_objects_projects.select(1).where('lfs_objects_projects.lfs_object_id = lfs_objects.id'))
@@ -37,13 +39,14 @@ class LfsObject < ApplicationRecord
     file_store == LfsObjectUploader::Store::LOCAL
   end
 
-  # rubocop: disable Cop/DestroyAll
-  def self.destroy_unreferenced
-    joins("LEFT JOIN lfs_objects_projects ON lfs_objects_projects.lfs_object_id = #{table_name}.id")
-        .where(lfs_objects_projects: { id: nil })
-        .destroy_all
+  def self.unreferenced_in_batches
+    each_batch(of: BATCH_SIZE, order: :desc) do |lfs_objects|
+      relation = lfs_objects.where('NOT EXISTS (?)',
+              LfsObjectsProject.select(1).where('lfs_objects_projects.lfs_object_id = lfs_objects.id'))
+
+      yield relation if relation.any?
+    end
   end
-  # rubocop: enable Cop/DestroyAll
 
   def self.calculate_oid(path)
     self.hexdigest(path)

app/workers/all_queues.yml

@@ -467,7 +467,7 @@
   :urgency: :low
   :resource_boundary: :unknown
   :weight: 1
-  :idempotent:
+  :idempotent: true
   :tags: []
 - :name: cronjob:repository_archive_cache
   :worker_name: RepositoryArchiveCacheWorker

app/workers/remove_unreferenced_lfs_objects_worker.rb

 # frozen_string_literal: true
 
-class RemoveUnreferencedLfsObjectsWorker # rubocop:disable Scalability/IdempotentWorker
+class RemoveUnreferencedLfsObjectsWorker
   include ApplicationWorker
 
   sidekiq_options retry: 3
@@ -10,8 +10,16 @@ class RemoveUnreferencedLfsObjectsWorker # rubocop:disable Scalability/Idempoten
   # rubocop:enable Scalability/CronWorkerContext
 
   feature_category :git_lfs
+  deduplicate :until_executed
+  idempotent!
 
   def perform
-    LfsObject.destroy_unreferenced
+    number_of_removed_files = 0
+
+    LfsObject.unreferenced_in_batches do |lfs_objects_without_projects|
+      number_of_removed_files += lfs_objects_without_projects.destroy_all.count # rubocop: disable Cop/DestroyAll
+    end
+
+    number_of_removed_files
   end
 end

lib/tasks/gitlab/cleanup.rake

@@ -92,9 +92,9 @@ namespace :gitlab do
     task orphan_lfs_files: :gitlab_environment do
       warn_user_is_not_gitlab
 
-      removed_files = RemoveUnreferencedLfsObjectsWorker.new.perform
+      number_of_removed_files = RemoveUnreferencedLfsObjectsWorker.new.perform
 
-      logger.info "Removed unreferenced LFS files: #{removed_files.count}".color(:green)
+      logger.info "Removed unreferenced LFS files: #{number_of_removed_files}".color(:green)
     end
 
     namespace :sessions do

spec/models/lfs_object_spec.rb

@@ -191,4 +191,21 @@ RSpec.describe LfsObject do
       expect { lfs_object.destroy! }.not_to raise_error
     end
   end
+
+  describe '.unreferenced_in_batches' do
+    let!(:unreferenced_lfs_object1) { create(:lfs_object, oid: '1') }
+    let!(:referenced_lfs_object) { create(:lfs_objects_project).lfs_object }
+    let!(:unreferenced_lfs_object2) { create(:lfs_object, oid: '2') }
+
+    it 'returns lfs objects in batches' do
+      stub_const('LfsObject::BATCH_SIZE', 1)
+
+      batches = []
+      described_class.unreferenced_in_batches { |batch| batches << batch }
+
+      expect(batches.size).to eq(2)
+      expect(batches.first).to eq([unreferenced_lfs_object2])
+      expect(batches.last).to eq([unreferenced_lfs_object1])
+    end
+  end
 end

spec/workers/remove_unreferenced_lfs_objects_worker_spec.rb

@@ -34,14 +34,14 @@ RSpec.describe RemoveUnreferencedLfsObjectsWorker do
     end
 
     it 'removes unreferenced lfs objects' do
-      worker.perform
+      expect(worker.perform).to eq(2)
 
       expect(LfsObject.where(id: unreferenced_lfs_object1.id)).to be_empty
       expect(LfsObject.where(id: unreferenced_lfs_object2.id)).to be_empty
     end
 
     it 'leaves referenced lfs objects' do
-      worker.perform
+      expect(worker.perform).to eq(2)
 
       expect(referenced_lfs_object1.reload).to be_present
       expect(referenced_lfs_object2.reload).to be_present
@@ -50,10 +50,12 @@ RSpec.describe RemoveUnreferencedLfsObjectsWorker do
     it 'removes unreferenced lfs objects after project removal' do
       project1.destroy!
 
-      worker.perform
+      expect(worker.perform).to eq(3)
 
       expect(referenced_lfs_object1.reload).to be_present
       expect(LfsObject.where(id: referenced_lfs_object2.id)).to be_empty
     end
   end
+
+  it_behaves_like 'an idempotent worker'
 end