Merge branch '7841-geo-deleting-a-project-leaves-orphaned-lfs-objects-around' into 'master'

Geo: Deleting a project leaves orphaned LFS objects and Job artifacts around

Closes #7841

See merge request gitlab-org/gitlab-ee!8031

ee/app/services/geo/file_registry_removal_service.rb

@@ -6,6 +6,15 @@ module Geo
 
     LEASE_TIMEOUT = 8.hours.freeze
 
+    # It's possible that LfsObject or Ci::JobArtifact record does not exists anymore
+    # In this case, you need to pass file_path parameter explicitly
+    #
+    def initialize(object_type, object_db_id, file_path = nil)
+      @object_type = object_type.to_sym
+      @object_db_id = object_db_id
+      @object_file_path = file_path
+    end
+
     def execute
       log_info('Executing')
 
@@ -48,6 +57,7 @@ module Geo
 
     def file_path
       strong_memoize(:file_path) do
+        next @object_file_path if @object_file_path
         # When local storage is used, just rely on the existing methods
         next file_uploader.file.path if file_uploader.object_store == ObjectStorage::Store::LOCAL
 

ee/app/workers/geo/file_registry_removal_worker.rb

@@ -4,10 +4,10 @@ module Geo
     include GeoQueue
     include ::Gitlab::Geo::LogHelpers
 
-    def perform(object_type, object_db_id)
-      log_info('Executing Geo::FileRegistryRemovalService', id: object_db_id, type: object_type)
+    def perform(object_type, object_db_id, file_path = nil)
+      log_info('Executing Geo::FileRegistryRemovalService', id: object_db_id, type: object_type, file_path: file_path)
 
-      ::Geo::FileRegistryRemovalService.new(object_type, object_db_id).execute
+      ::Geo::FileRegistryRemovalService.new(object_type, object_db_id, file_path).execute
     end
   end
 end

ee/changelogs/unreleased/7841-geo-deleting-a-project-leaves-orphaned-lfs-objects-around.yml

+---
+title: '[Geo] Fix: Deleting a project leaves orphaned LFS objects and CI Job artifacts around'
+merge_request: 8031
+author:
+type: fixed

ee/lib/gitlab/geo/log_cursor/events/job_artifact_deleted_event.rb

@@ -7,12 +7,16 @@ module Gitlab
 
           def process
             # Must always schedule, regardless of shard health
-            job_id = ::Geo::FileRegistryRemovalWorker.perform_async(:job_artifact, event.job_artifact_id)
+            job_id = ::Geo::FileRegistryRemovalWorker.perform_async(:job_artifact, event.job_artifact_id, file_path)
             log_event(job_id)
           end
 
           private
 
+          def file_path
+            @file_path ||= File.join(::JobArtifactUploader.root, event.file_path)
+          end
+
           def log_event(job_id)
             logger.event_info(
               created_at,

ee/lib/gitlab/geo/log_cursor/events/lfs_object_deleted_event.rb

@@ -7,12 +7,16 @@ module Gitlab
 
           def process
             # Must always schedule, regardless of shard health
-            job_id = ::Geo::FileRegistryRemovalWorker.perform_async(:lfs, event.lfs_object_id)
+            job_id = ::Geo::FileRegistryRemovalWorker.perform_async(:lfs, event.lfs_object_id, file_path)
             log_event(job_id)
           end
 
           private
 
+          def file_path
+            @file_path ||= File.join(LfsObjectUploader.root, event.file_path)
+          end
+
           def log_event(job_id)
             logger.event_info(
               created_at,

ee/spec/lib/gitlab/geo/log_cursor/events/job_artifact_deleted_event_spec.rb

@@ -13,6 +13,12 @@ describe Gitlab::Geo::LogCursor::Events::JobArtifactDeletedEvent, :postgresql, :
     Sidekiq::Testing.inline! { example.run }
   end
 
+  it 'schedules a Geo::FileRegistryRemovalWorker job' do
+    expect(::Geo::FileRegistryRemovalWorker).to receive(:perform_async).with(:job_artifact, job_artifact_deleted_event.job_artifact_id, job_artifact.file.path)
+
+    subject.process
+  end
+
   describe '#process' do
     context 'with a tracking database entry' do
       before do

ee/spec/lib/gitlab/geo/log_cursor/events/lfs_object_deleted_event_spec.rb

@@ -25,7 +25,7 @@ describe Gitlab::Geo::LogCursor::Events::LfsObjectDeletedEvent, :postgresql, :cl
     end
 
     it 'schedules a Geo::FileRegistryRemovalWorker job' do
-      expect(::Geo::FileRegistryRemovalWorker).to receive(:perform_async).with(:lfs, lfs_object_deleted_event.lfs_object_id)
+      expect(::Geo::FileRegistryRemovalWorker).to receive(:perform_async).with(:lfs, lfs_object_deleted_event.lfs_object_id, lfs_object.file.path)
 
       subject.process
     end

ee/spec/services/geo/file_registry_removal_service_spec.rb

@@ -76,6 +76,16 @@ describe Geo::FileRegistryRemovalService do
 
         it_behaves_like 'removes'
       end
+
+      context 'no lfs_object record' do
+        before do
+          lfs_object.delete
+        end
+
+        it_behaves_like 'removes' do
+          subject(:service) { described_class.new(file_registry.file_type, file_registry.file_id, file_path) }
+        end
+      end
     end
 
     context 'with job artifact' do
@@ -93,6 +103,16 @@ describe Geo::FileRegistryRemovalService do
 
         it_behaves_like 'removes artifact'
       end
+
+      context 'no job artifact record' do
+        before do
+          job_artifact.delete
+        end
+
+        it_behaves_like 'removes artifact' do
+          subject(:service) { described_class.new('job_artifact', registry.artifact_id, file_path) }
+        end
+      end
     end
 
     context 'with avatar' do