Exclude expired artifacts from syncable

ee/app/models/ee/ci/job_artifact.rb

@@ -9,7 +9,8 @@ module EE
     prepended do
       after_destroy :log_geo_event
 
-      scope :geo_syncable, -> { with_files_stored_locally }
+      scope :not_expired, -> { where('expire_at IS NULL OR expire_at > ?', Time.current) }
+      scope :geo_syncable, -> { with_files_stored_locally.not_expired }
     end
 
     private

ee/app/models/geo/fdw/ci/job_artifact.rb

@@ -6,7 +6,8 @@ module Geo
 
         scope :with_files_stored_locally, -> { where(file_store: [nil, JobArtifactUploader::Store::LOCAL]) }
         scope :with_files_stored_remotely, -> { where(file_store: JobArtifactUploader::Store::REMOTE) }
-        scope :geo_syncable, -> { with_files_stored_locally }
+        scope :not_expired, -> { where('expire_at IS NULL OR expire_at > ?', Time.current) }
+        scope :geo_syncable, -> { with_files_stored_locally.not_expired }
       end
     end
   end

ee/changelogs/unreleased/mk-geo-exclude-expired-artifacts.yml

+---
+title: 'Geo: Exclude expired job artifacts from syncing and counts'
+merge_request: 5380
+author:
+type: fixed

ee/spec/finders/geo/job_artifact_registry_finder_spec.rb

@@ -41,6 +41,12 @@ describe Geo::JobArtifactRegistryFinder, :geo do
         expect(subject.count_syncable_job_artifacts).to eq 3
       end
 
+      it 'ignores expired job artifacts' do
+        job_artifact_1.update_column(:expire_at, Date.yesterday)
+
+        expect(subject.count_syncable_job_artifacts).to eq 3
+      end
+
       context 'with selective sync' do
         before do
           secondary.update!(selective_sync_type: 'namespaces', namespaces: [synced_group])
@@ -55,6 +61,12 @@ describe Geo::JobArtifactRegistryFinder, :geo do
 
           expect(subject.count_syncable_job_artifacts).to eq 1
         end
+
+        it 'ignores expired job artifacts' do
+          job_artifact_1.update_column(:expire_at, Date.yesterday)
+
+          expect(subject.count_syncable_job_artifacts).to eq 1
+        end
       end
     end
 
@@ -91,6 +103,15 @@ describe Geo::JobArtifactRegistryFinder, :geo do
         expect(subject.count_synced_job_artifacts).to eq 2
       end
 
+      it 'ignores expired job artifacts' do
+        job_artifact_1.update_column(:expire_at, Date.yesterday)
+        create(:geo_job_artifact_registry, artifact_id: job_artifact_1.id)
+        create(:geo_job_artifact_registry, artifact_id: job_artifact_2.id)
+        create(:geo_job_artifact_registry, artifact_id: job_artifact_3.id)
+
+        expect(subject.count_synced_job_artifacts).to eq 2
+      end
+
       context 'with selective sync' do
         before do
           secondary.update!(selective_sync_type: 'namespaces', namespaces: [synced_group])
@@ -117,6 +138,15 @@ describe Geo::JobArtifactRegistryFinder, :geo do
 
           expect(subject.count_synced_job_artifacts).to eq 1
         end
+
+        it 'ignores expired job artifacts' do
+          job_artifact_1.update_column(:expire_at, Date.yesterday)
+          create(:geo_job_artifact_registry, artifact_id: job_artifact_1.id)
+          create(:geo_job_artifact_registry, artifact_id: job_artifact_2.id)
+          create(:geo_job_artifact_registry, artifact_id: job_artifact_3.id)
+
+          expect(subject.count_synced_job_artifacts).to eq 1
+        end
       end
     end
 
@@ -153,6 +183,15 @@ describe Geo::JobArtifactRegistryFinder, :geo do
         expect(subject.count_failed_job_artifacts).to eq 2
       end
 
+      it 'ignores expired job artifacts' do
+        job_artifact_1.update_column(:expire_at, Date.yesterday)
+        create(:geo_job_artifact_registry, artifact_id: job_artifact_1.id, success: false)
+        create(:geo_job_artifact_registry, artifact_id: job_artifact_2.id, success: false)
+        create(:geo_job_artifact_registry, artifact_id: job_artifact_3.id, success: false)
+
+        expect(subject.count_failed_job_artifacts).to eq 2
+      end
+
       context 'with selective sync' do
         before do
           secondary.update!(selective_sync_type: 'namespaces', namespaces: [synced_group])
@@ -178,10 +217,19 @@ describe Geo::JobArtifactRegistryFinder, :geo do
         end
 
         it 'ignores remote job artifacts' do
+          job_artifact_1.update_column(:file_store, ObjectStorage::Store::REMOTE)
+          create(:geo_job_artifact_registry, artifact_id: job_artifact_1.id, success: false)
+          create(:geo_job_artifact_registry, artifact_id: job_artifact_2.id, success: false)
+          create(:geo_job_artifact_registry, artifact_id: job_artifact_3.id, success: false)
+
+          expect(subject.count_failed_job_artifacts).to eq 1
+        end
+
+        it 'ignores expired job artifacts' do
+          job_artifact_1.update_column(:expire_at, Date.yesterday)
           create(:geo_job_artifact_registry, artifact_id: job_artifact_1.id, success: false)
           create(:geo_job_artifact_registry, artifact_id: job_artifact_2.id, success: false)
           create(:geo_job_artifact_registry, artifact_id: job_artifact_3.id, success: false)
-          job_artifact_1.update_column(:file_store, ObjectStorage::Store::REMOTE)
 
           expect(subject.count_failed_job_artifacts).to eq 1
         end
@@ -229,6 +277,13 @@ describe Geo::JobArtifactRegistryFinder, :geo do
         expect(subject.count_synced_missing_on_primary_job_artifacts).to eq 0
       end
 
+      it 'ignores expired job artifacts' do
+        job_artifact_1.update_column(:expire_at, Date.yesterday)
+        create(:geo_job_artifact_registry, artifact_id: job_artifact_1.id, missing_on_primary: true)
+
+        expect(subject.count_synced_missing_on_primary_job_artifacts).to eq 0
+      end
+
       context 'with selective sync' do
         before do
           secondary.update!(selective_sync_type: 'namespaces', namespaces: [synced_group])
@@ -253,6 +308,13 @@ describe Geo::JobArtifactRegistryFinder, :geo do
 
           expect(subject.count_synced_missing_on_primary_job_artifacts).to eq 0
         end
+
+        it 'ignores expired job artifacts' do
+          job_artifact_1.update_column(:expire_at, Date.yesterday)
+          create(:geo_job_artifact_registry, artifact_id: job_artifact_1.id, missing_on_primary: true)
+
+          expect(subject.count_synced_missing_on_primary_job_artifacts).to eq 0
+        end
       end
     end
   end
@@ -282,6 +344,22 @@ describe Geo::JobArtifactRegistryFinder, :geo do
 
         expect(job_artifacts).to match_ids(job_artifact_4)
       end
+
+      it 'ignores remote job artifacts' do
+        job_artifact_2.update_column(:file_store, ObjectStorage::Store::REMOTE)
+
+        job_artifacts = subject.find_unsynced_job_artifacts(batch_size: 10)
+
+        expect(job_artifacts).to match_ids(job_artifact_4)
+      end
+
+      it 'ignores expired job artifacts' do
+        job_artifact_2.update_column(:expire_at, Date.yesterday)
+
+        job_artifacts = subject.find_unsynced_job_artifacts(batch_size: 10)
+
+        expect(job_artifacts).to match_ids(job_artifact_4)
+      end
     end
 
     describe '#find_migrated_local_job_artifacts' do
@@ -324,6 +402,15 @@ describe Geo::JobArtifactRegistryFinder, :geo do
 
         expect(job_artifacts).to match_ids(job_artifact_remote_2)
       end
+
+      it 'includes synced job artifacts that are expired' do
+        job_artifact = create(:ci_job_artifact, :remote_store, project: synced_project, expire_at: Date.yesterday)
+        create(:geo_job_artifact_registry, artifact_id: job_artifact.id)
+
+        job_artifacts = subject.find_migrated_local_job_artifacts(batch_size: 10)
+
+        expect(job_artifacts).to match_ids(job_artifact)
+      end
     end
   end