Merge branch...

Merge branch '222495-geo-geo-job-artifact-registry-finder-find_registry_differences-should-ignore-expired-ones' into 'master'

Geo - Does not sync expired job artifacts

Closes #222495

See merge request gitlab-org/gitlab!35082

ee/app/finders/geo/job_artifact_registry_finder.rb

@@ -44,7 +44,7 @@ module Geo
     #
     # @return [Array] the first element is an Array of untracked IDs, and the second element is an Array of tracked IDs that are unused
     def find_registry_differences(range)
-      source_ids = job_artifacts.id_in(range).pluck(::Ci::JobArtifact.arel_table[:id]) # rubocop:disable CodeReuse/ActiveRecord
+      source_ids = job_artifacts.not_expired.id_in(range).pluck(::Ci::JobArtifact.arel_table[:id]) # rubocop:disable CodeReuse/ActiveRecord
       tracked_ids = syncable.pluck_model_ids_in_range(range)
 
       untracked_ids = source_ids - tracked_ids

ee/changelogs/unreleased/222495-geo-geo-job-artifact-registry-finder-find_registry_differences-.yml

+---
+title: Geo - Does not sync expired job artifacts
+merge_request: 35082
+author:
+type: fixed

ee/spec/finders/geo/job_artifact_registry_finder_spec.rb

@@ -106,6 +106,12 @@ RSpec.describe Geo::JobArtifactRegistryFinder, :geo do
   end
 
   describe '#find_registry_differences' do
+    # Untracked IDs should not contain any of these expired job artifacts.
+    let!(:ci_job_artifact_6) { create(:ci_job_artifact, :expired, project: synced_project) }
+    let!(:ci_job_artifact_7) { create(:ci_job_artifact, :expired, project: unsynced_project) }
+    let!(:ci_job_artifact_8) { create(:ci_job_artifact, :expired, project: project_broken_storage) }
+    let!(:ci_job_artifact_remote_4) { create(:ci_job_artifact, :expired, :remote_store) }
+
     context 'untracked IDs' do
       before do
         create(:geo_job_artifact_registry, artifact_id: ci_job_artifact_1.id)
@@ -181,6 +187,26 @@ RSpec.describe Geo::JobArtifactRegistryFinder, :geo do
         end
       end
 
+      context 'with an expired registry' do
+        let!(:expired) { create(:geo_job_artifact_registry, artifact_id: ci_job_artifact_6.id) }
+
+        it 'includes expired tracked IDs that exists in the model table' do
+          range = ci_job_artifact_6.id..ci_job_artifact_6.id
+
+          _, unused_tracked_ids = subject.find_registry_differences(range)
+
+          expect(unused_tracked_ids).to match_array([ci_job_artifact_6.id])
+        end
+
+        it 'excludes IDs outside the ID range' do
+          range = (ci_job_artifact_6.id + 1)..(ci_job_artifact_6.id + 10)
+
+          _, unused_tracked_ids = subject.find_registry_differences(range)
+
+          expect(unused_tracked_ids).to be_empty
+        end
+      end
+
       context 'with selective sync by namespace' do
         let(:secondary) { create(:geo_node, selective_sync_type: 'namespaces', namespaces: [synced_group]) }
 
@@ -204,6 +230,16 @@ RSpec.describe Geo::JobArtifactRegistryFinder, :geo do
 
               expect(unused_tracked_ids).to be_empty
             end
+
+            it 'includes expired tracked IDs that are in selectively synced projects' do
+              create(:geo_job_artifact_registry, artifact_id: ci_job_artifact_6.id)
+
+              range = ci_job_artifact_6.id..ci_job_artifact_6.id
+
+              _, unused_tracked_ids = subject.find_registry_differences(range)
+
+              expect(unused_tracked_ids).to match_array([ci_job_artifact_6.id])
+            end
           end
         end
       end
@@ -231,6 +267,16 @@ RSpec.describe Geo::JobArtifactRegistryFinder, :geo do
 
               expect(unused_tracked_ids).to be_empty
             end
+
+            it 'includes expired tracked IDs that are in selectively synced shards' do
+              create(:geo_job_artifact_registry, artifact_id: ci_job_artifact_8.id)
+
+              range = ci_job_artifact_8.id..ci_job_artifact_8.id
+
+              _, unused_tracked_ids = subject.find_registry_differences(range)
+
+              expect(unused_tracked_ids).to match_array([ci_job_artifact_8.id])
+            end
           end
         end
       end
@@ -248,6 +294,16 @@ RSpec.describe Geo::JobArtifactRegistryFinder, :geo do
 
               expect(unused_tracked_ids).to match_array([ci_job_artifact_remote_1.id])
             end
+
+            it 'includes expired tracked IDs that are in object storage' do
+              create(:geo_job_artifact_registry, artifact_id: ci_job_artifact_remote_4.id)
+
+              range = ci_job_artifact_remote_4.id..ci_job_artifact_remote_4.id
+
+              _, unused_tracked_ids = subject.find_registry_differences(range)
+
+              expect(unused_tracked_ids).to match_array([ci_job_artifact_remote_4.id])
+            end
           end
 
           context 'not in object storage' do