Merge branch 'mk/fix-blob-delete-events' into 'master'

Geo: Avoid orphaning blob files on secondaries

Closes #233040

See merge request gitlab-org/gitlab!41529

ee/app/models/concerns/geo/blob_replicator_strategy.rb

@@ -34,8 +34,6 @@ module Geo
 
     # Called by Gitlab::Geo::Replicator#consume
     def consume_event_deleted(**params)
-      return unless in_replicables_for_geo_node?
-
       replicate_destroy(params)
     end
 
@@ -51,7 +49,7 @@ module Geo
     #
     # @return [String] File path
     def blob_path
-      carrierwave_uploader.class.absolute_path(carrierwave_uploader)
+      carrierwave_uploader.path
     end
 
     def calculate_checksum!
@@ -100,7 +98,7 @@ module Geo
     def replicate_destroy(event_data)
       ::Geo::FileRegistryRemovalService.new(
         replicable_name,
-        model_record.id,
+        model_record_id,
         event_data[:blob_path]
       ).execute
     end

ee/changelogs/unreleased/mk-fix-blob-delete-events.yml

+---
+title: 'Geo: Avoid orphaning blob files on secondaries'
+merge_request: 41529
+author:
+type: fixed

ee/spec/support/shared_examples/models/concerns/blob_replicator_strategy_shared_examples.rb

@@ -131,26 +131,30 @@ RSpec.shared_examples 'a blob replicator' do
   end
 
   describe '#consume_event_deleted' do
-    context "when the blob's project is in replicables for this geo node" do
+    let!(:model_record_id) { replicator.model_record_id }
+    let!(:blob_path) { replicator.blob_path }
+    let!(:deleted_params) { { model_record_id: model_record_id, blob_path: blob_path } }
+
+    context 'when model_record was deleted from the DB and the replicator only has its ID' do
+      before do
+        model_record.delete
+      end
+
+      # The replicator is instantiated by Geo::EventService on the secondary
+      # side, after the model_record no longer exists. This line ensures the
+      # replicator does not hold an instance of ActiveRecord::Base, which helps
+      # avoid a regression of
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/233040
+      let(:secondary_side_replicator) { replicator.class.new(model_record: nil, model_record_id: model_record_id) }
+
       it 'invokes Geo::FileRegistryRemovalService' do
-        expect(replicator).to receive(:in_replicables_for_geo_node?).and_return(true)
         service = double(:service)
 
         expect(service).to receive(:execute)
         expect(::Geo::FileRegistryRemovalService)
-          .to receive(:new).with(replicator.replicable_name, replicator.model_record_id, 'blob_path').and_return(service)
+          .to receive(:new).with(secondary_side_replicator.replicable_name, model_record_id, blob_path).and_return(service)
 
-        replicator.consume_event_deleted({ blob_path: 'blob_path' })
-      end
-    end
-
-    context "when the blob's project is not in replicables for this geo node" do
-      it 'does not invoke Geo::FileRegistryRemovalService' do
-        expect(replicator).to receive(:in_replicables_for_geo_node?).and_return(false)
-
-        expect(::Geo::FileRegistryRemovalService).not_to receive(:new)
-
-        replicator.consume_event_deleted({ blob_path: '' })
+        secondary_side_replicator.consume_event_deleted(deleted_params)
       end
     end
   end
@@ -176,4 +180,14 @@ RSpec.shared_examples 'a blob replicator' do
       expect(invoke_model).to be_a(Class)
     end
   end
+
+  describe '#blob_path' do
+    context 'when the file is locally stored' do
+      it 'returns a valid path to a file' do
+        file_exist = File.exist?(replicator.blob_path)
+
+        expect(file_exist).to be_truthy
+      end
+    end
+  end
 end