Use run_after_commit on object to schedule background upload

app/models/ci/job_artifact.rb

@@ -10,6 +10,12 @@ module Ci
 
     mount_uploader :file, JobArtifactUploader
 
+    after_save if: :file_changed?, on: [:create, :update] do
+      run_after_commit do
+        file.schedule_migration_to_object_storage
+      end
+    end
+
     enum file_type: {
       archive: 1,
       metadata: 2

app/models/lfs_object.rb

@@ -11,6 +11,12 @@ class LfsObject < ActiveRecord::Base
 
   mount_uploader :file, LfsObjectUploader
 
+  after_save if: :file_changed?, on: [:create, :update] do
+    run_after_commit do
+      file.schedule_migration_to_object_storage
+    end
+  end
+
   def project_allowed_access?(project)
     projects.exists?(project.lfs_storage_project.id)
   end

app/uploaders/job_artifact_uploader.rb

 class JobArtifactUploader < ObjectStoreUploader
   storage_options Gitlab.config.artifacts
-  after :store, :schedule_migration_to_object_storage
 
   def self.local_store_path
     Gitlab.config.artifacts.path

app/uploaders/lfs_object_uploader.rb

 class LfsObjectUploader < ObjectStoreUploader
   storage_options Gitlab.config.lfs
-  after :store, :schedule_migration_to_object_storage
 
   def self.local_store_path
     Gitlab.config.lfs.storage_path

ee/app/uploaders/object_store_uploader.rb

@@ -117,14 +117,12 @@ class ObjectStoreUploader < CarrierWave::Uploader::Base
   end
 
   def schedule_migration_to_object_storage(*args)
-    if self.class.object_store_enabled? && licensed? && file_storage?
-      uploader = self
-      mount_field = mounted_as
+    return unless self.class.object_store_enabled?
+    return unless self.class.background_upload_enabled?
+    return unless self.licensed?
+    return unless self.file_storage?
 
-      model.run_after_commit do
-        ObjectStorageUploadWorker.perform_async(uploader.class.name, self.class.name, mount_field, id)
-      end
-    end
+    ObjectStorageUploadWorker.perform_async(self.class.name, model.class.name, mounted_as, model.id)
   end
 
   def fog_directory

spec/ee/spec/models/ee/lfs_object_spec.rb

@@ -33,4 +33,64 @@ describe LfsObject do
       end
     end
   end
+
+  describe '#schedule_migration_to_object_storage' do
+    before do
+      stub_lfs_setting(enabled: true)
+    end
+
+    subject { create(:lfs_object, :with_file) }
+
+    context 'when object storage is disabled' do
+      before do
+        stub_lfs_object_storage(enabled: false)
+      end
+
+      it 'does not schedule the migration' do
+        expect(ObjectStorageUploadWorker).not_to receive(:perform_async)
+
+        subject
+      end
+    end
+
+    context 'when object storage is enabled' do
+      context 'when background upload is enabled' do
+        context 'when is licensed' do
+          before do
+            stub_lfs_object_storage(background_upload: true)
+          end
+
+          it 'schedules the model for migration' do
+            expect(ObjectStorageUploadWorker).to receive(:perform_async).with('LfsObjectUploader', described_class.name, :file, kind_of(Numeric))
+
+            subject
+          end
+        end
+
+        context 'when is unlicensed' do
+          before do
+            stub_lfs_object_storage(background_upload: true, licensed: false)
+          end
+
+          it 'does not schedule the migration' do
+            expect(ObjectStorageUploadWorker).not_to receive(:perform_async)
+
+            subject
+          end
+        end
+      end
+
+      context 'when background upload is disabled' do
+        before do
+          stub_lfs_object_storage(background_upload: false)
+        end
+
+        it 'schedules the model for migration' do
+          expect(ObjectStorageUploadWorker).not_to receive(:perform_async)
+
+          subject
+        end
+      end
+    end
+  end
 end

spec/models/ci/job_artifact_spec.rb

@@ -17,6 +17,10 @@ describe Ci::JobArtifact do
 
     describe '#schedule_migration_to_object_storage' do
       context 'when object storage is disabled' do
+        before do
+          stub_artifacts_object_storage(enabled: false)
+        end
+
         it 'does not schedule the migration' do
           expect(ObjectStorageUploadWorker).not_to receive(:perform_async)
 
@@ -25,26 +29,42 @@ describe Ci::JobArtifact do
       end
 
       context 'when object storage is enabled' do
-        before do
-          stub_artifacts_object_storage
-        end
-
-        it 'schedules the model for migration' do
-          expect(ObjectStorageUploadWorker).to receive(:perform_async).with('JobArtifactUploader', described_class.name, :file, kind_of(Numeric))
-
-          subject
+        context 'when background upload is enabled' do
+          context 'when is licensed' do
+            before do
+              stub_artifacts_object_storage(background_upload: true)
+            end
+
+            it 'schedules the model for migration' do
+              expect(ObjectStorageUploadWorker).to receive(:perform_async).with('JobArtifactUploader', described_class.name, :file, kind_of(Numeric))
+
+              subject
+            end
+          end
+
+          context 'when is unlicensed' do
+            before do
+              stub_artifacts_object_storage(background_upload: true, licensed: false)
+            end
+
+            it 'does not schedule the migration' do
+              expect(ObjectStorageUploadWorker).not_to receive(:perform_async)
+
+              subject
+            end
+          end
         end
-      end
 
-      context 'when object storage is unlicensed' do
-        before do
-          stub_artifacts_object_storage(licensed: false)
-        end
+        context 'when background upload is disabled' do
+          before do
+            stub_artifacts_object_storage(background_upload: false)
+          end
 
-        it 'does not schedule the migration' do
-          expect(ObjectStorageUploadWorker).not_to receive(:perform_async)
+          it 'schedules the model for migration' do
+            expect(ObjectStorageUploadWorker).not_to receive(:perform_async)
 
-          subject
+            subject
+          end
         end
       end
     end

spec/requests/api/runner_spec.rb

@@ -1085,8 +1085,6 @@ describe API::Runner do
             let(:stored_artifacts_size) { job.reload.artifacts_size }
 
             before do
-              stub_artifacts_object_storage(enabled: false)
-
               post(api("/jobs/#{job.id}/artifacts"), post_data, headers_with_token)
             end
 
@@ -1156,6 +1154,13 @@ describe API::Runner do
 
         context 'when job has artifacts' do
           let(:job) { create(:ci_build) }
+          let(:store) { JobArtifactUploader::LOCAL_STORE }
+
+          before do
+            create(:ci_job_artifact, :archive, file_store: store, job: job)
+
+            download_artifact
+          end
 
           context 'when using job token' do
             context 'when artifacts are stored locally' do
@@ -1165,11 +1170,6 @@ describe API::Runner do
               end
 
               it 'download artifacts' do
-                stub_artifacts_object_storage(enabled: false)
-                create(:ci_job_artifact, :archive, job: job)
-
-                download_artifact
-
                 expect(response).to have_gitlab_http_status(200)
                 expect(response.headers).to include download_headers
               end
@@ -1180,10 +1180,6 @@ describe API::Runner do
               let!(:job) { create(:ci_build) }
 
               it 'download artifacts' do
-                create(:ci_job_artifact, :archive, :remote_store, job: job)
-
-                download_artifact
-
                 expect(response).to have_gitlab_http_status(302)
               end
             end
@@ -1193,8 +1189,6 @@ describe API::Runner do
             let(:token) { job.project.runners_token }
 
             it 'responds with forbidden' do
-              download_artifact
-
               expect(response).to have_gitlab_http_status(403)
             end
           end

spec/requests/lfs_http_spec.rb

@@ -1038,7 +1038,7 @@ describe 'Git LFS API and storage' do
 
             context 'with object storage enabled' do
               before do
-                stub_lfs_object_storage
+                stub_lfs_object_storage(background_upload: true)
               end
 
               it 'schedules migration of file to object storage' do

spec/support/stub_object_storage.rb

 module StubConfiguration
-  def stub_object_storage_uploader(config:, uploader:, remote_directory:, enabled: true, licensed: true, background_upload: nil)
+  def stub_object_storage_uploader(config:, uploader:, remote_directory:, enabled: true, licensed: true, background_upload: false)
     Fog.mock!
 
     allow(config).to receive(:enabled) { enabled }
-    allow(config).to receive(:background_upload) { background_upload } if background_upload
+    allow(config).to receive(:background_upload) { background_upload }
 
     stub_licensed_features(object_storage: licensed) unless licensed == :skip
 

spec/uploaders/lfs_object_uploader_spec.rb

@@ -49,7 +49,7 @@ describe LfsObjectUploader do
 
     context 'with object storage enabled' do
       before do
-        stub_lfs_object_storage
+        stub_lfs_object_storage(background_upload: true)
       end
 
       it 'is scheduled to run after creation' do