raises error if another worker is already running and improves code quality

app/models/remote_mirror.rb

@@ -74,8 +74,7 @@ class RemoteMirror < ActiveRecord::Base
   end
 
   def sync
-    return unless project
-    return if !enabled || update_in_progress?
+    return unless project && enabled
 
     schedule_update_job
   end

app/workers/repository_update_remote_mirror_worker.rb

 class RepositoryUpdateRemoteMirrorWorker
-  UpdateRemoteMirrorError = Class.new(StandardError)
+  UpdateAlreadyInProgressError = Class.new(StandardError)
+  UpdateError = Class.new(StandardError)
 
   include Sidekiq::Worker
   include Gitlab::ShellAdapter
 
   sidekiq_options queue: :project_mirror, retry: 3
 
-  sidekiq_retries_exhausted do |msg, e|
+  sidekiq_retry_in { |count| 30 * count }
+
+  sidekiq_retries_exhausted do |msg, _|
     Sidekiq.logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}"
   end
 
   def perform(remote_mirror_id, scheduled_time)
-    begin
-      remote_mirror = RemoteMirror.find(remote_mirror_id)
-      return if remote_mirror.updated_since?(scheduled_time)
-
-      remote_mirror.update_start
-
-      project = remote_mirror.project
-      current_user = project.creator
-      result = Projects::UpdateRemoteMirrorService.new(project, current_user).execute(remote_mirror)
-
-      raise UpdateRemoteMirrorError, result[:message] if result[:status] == :error
-
-      remote_mirror.update_finish
-    rescue UpdateRemoteMirrorError => ex
-      remote_mirror.mark_as_failed(Gitlab::UrlSanitizer.sanitize(ex.message))
-      raise
-    rescue => ex
-      raise UpdateRemoteMirrorError, "#{ex.class}: #{ex.message}"
-    end
+    remote_mirror = RemoteMirror.find(remote_mirror_id)
+    return if remote_mirror.updated_since?(scheduled_time)
+
+    raise UpdateAlreadyInProgressError if remote_mirror.update_in_progress?
+    remote_mirror.update_start
+
+    project = remote_mirror.project
+    current_user = project.creator
+    result = Projects::UpdateRemoteMirrorService.new(project, current_user).execute(remote_mirror)
+
+    raise UpdateError, result[:message] if result[:status] == :error
+    remote_mirror.update_finish
+  rescue UpdateAlreadyInProgressError
+    raise
+  rescue UpdateError => ex
+    remote_mirror.mark_as_failed(Gitlab::UrlSanitizer.sanitize(ex.message))
+    raise
+  rescue => ex
+    raise UpdateError, "#{ex.class}: #{ex.message}"
   end
 end

spec/services/git_push_service_spec.rb

@@ -21,10 +21,11 @@ describe GitPushService, services: true do
     let(:ref)      { @ref }
 
     subject do
-      described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref )
+      described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
     end
 
     it 'fails stuck remote mirrors' do
+      allow(project).to receive(:update_remote_mirrors).and_return(project.remote_mirrors)
       expect(project).to receive(:mark_stuck_remote_mirrors_as_failed!)
 
       subject.execute

spec/workers/repository_update_remote_mirror_worker_spec.rb

@@ -11,7 +11,7 @@ describe RepositoryUpdateRemoteMirrorWorker do
   end
 
   describe '#perform' do
-    context 'with status scheduling' do
+    context 'with status none' do
       before do
         remote_mirror.update_attributes(update_status: 'none')
       end
@@ -23,11 +23,13 @@ describe RepositoryUpdateRemoteMirrorWorker do
       end
 
       it 'sets status as failed when update remote mirror service executes with errors' do
-        expect_any_instance_of(Projects::UpdateRemoteMirrorService).to receive(:execute).with(remote_mirror).and_return(status: :error, message: 'fail!')
+        error_message = 'fail!'
 
+        expect_any_instance_of(Projects::UpdateRemoteMirrorService).to receive(:execute).with(remote_mirror).and_return(status: :error, message: error_message)
         expect do
           subject.perform(remote_mirror.id, Time.now)
-        end.to raise_error
+        end.to raise_error(RepositoryUpdateRemoteMirrorWorker::UpdateError, 'fail!')
+
         expect(remote_mirror.reload.update_status).to eq('failed')
       end
 
@@ -41,6 +43,18 @@ describe RepositoryUpdateRemoteMirrorWorker do
       end
     end
 
+    context 'with another worker already running' do
+      before do
+        remote_mirror.update_attributes(update_status: 'started')
+      end
+
+      it 'raises RemoteMirrorUpdateAlreadyInProgressError' do
+        expect do
+          subject.perform(remote_mirror.id, Time.now)
+        end.to raise_error(RepositoryUpdateRemoteMirrorWorker::UpdateAlreadyInProgressError)
+      end
+    end
+
     context 'with status failed' do
       before do
         remote_mirror.update_attributes(update_status: 'failed')