Properly handle `sidekiq` skipsidekiq-interrupt-running-jobs

Transform `CancelledError` into `JobRetry::Skip`

5 files changed, 31 insertions, 17 deletions

diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb
index e145af5e2d5..9f3e104bc2b 100644
--- a/config/initializers/sidekiq.rb
+++ b/config/initializers/sidekiq.rb
@@ -28,12 +28,13 @@ if Rails.env.development?
 end
 
 enable_json_logs = Gitlab.config.sidekiq.log_format == 'json'
+enable_sidekiq_monitor = ENV.fetch("SIDEKIQ_MONITOR_WORKER", 0).to_i.nonzero?
 
 Sidekiq.configure_server do |config|
   config.redis = queues_config_hash
 
   config.server_middleware do |chain|
-    chain.add Gitlab::SidekiqMiddleware::Monitor
+    chain.add Gitlab::SidekiqMiddleware::Monitor if enable_sidekiq_monitor
     chain.add Gitlab::SidekiqMiddleware::Metrics if Settings.monitoring.sidekiq_exporter
     chain.add Gitlab::SidekiqMiddleware::ArgumentsLogger if ENV['SIDEKIQ_LOG_ARGUMENTS'] && !enable_json_logs
     chain.add Gitlab::SidekiqMiddleware::MemoryKiller if ENV['SIDEKIQ_MEMORY_KILLER_MAX_RSS']
@@ -59,9 +60,7 @@ Sidekiq.configure_server do |config|
     # Sidekiq (e.g. in an initializer).
     ActiveRecord::Base.clear_all_connections!
 
-    if ENV.fetch("SIDEKIQ_MONITOR_WORKER", 0).to_i.nonzero?
-      Gitlab::SidekiqMonitor.instance.start
-    end
+    Gitlab::SidekiqMonitor.instance.start if enable_sidekiq_monitor
   end
 
   if enable_reliable_fetch?
diff --git a/lib/gitlab/sidekiq_middleware/monitor.rb b/lib/gitlab/sidekiq_middleware/monitor.rb
index 2d0e5a6d635..0d88fe760d3 100644
--- a/lib/gitlab/sidekiq_middleware/monitor.rb
+++ b/lib/gitlab/sidekiq_middleware/monitor.rb
@@ -7,6 +7,9 @@ module Gitlab
         Gitlab::SidekiqMonitor.instance.within_job(job['jid'], queue) do
           yield
         end
+      rescue Gitlab::SidekiqMonitor::CancelledError
+        # ignore retries
+        raise Sidekiq::JobRetry::Skip
       end
     end
   end
diff --git a/lib/gitlab/sidekiq_monitor.rb b/lib/gitlab/sidekiq_monitor.rb
index 9ec3be48adb..9842f1f53f7 100644
--- a/lib/gitlab/sidekiq_monitor.rb
+++ b/lib/gitlab/sidekiq_monitor.rb
@@ -30,7 +30,7 @@ module Gitlab
 
       if cancelled?(jid)
         Sidekiq.logger.warn(
-          class: self.class,
+          class: self.class.to_s,
           action: 'run',
           queue: queue,
           jid: jid,
@@ -62,7 +62,7 @@ module Gitlab
 
     def start_working
       Sidekiq.logger.info(
-        class: self.class,
+        class: self.class.to_s,
         action: 'start',
         message: 'Starting Monitor Daemon'
       )
@@ -74,7 +74,7 @@ module Gitlab
 
     ensure
       Sidekiq.logger.warn(
-        class: self.class,
+        class: self.class.to_s,
         action: 'stop',
         message: 'Stopping Monitor Daemon'
       )
@@ -94,7 +94,7 @@ module Gitlab
       end
     rescue Exception => e # rubocop:disable Lint/RescueException
       Sidekiq.logger.warn(
-        class: self.class,
+        class: self.class.to_s,
         action: 'exception',
         message: e.message
       )
@@ -105,7 +105,7 @@ module Gitlab
 
     def process_message(message)
       Sidekiq.logger.info(
-        class: self.class,
+        class: self.class.to_s,
         channel: NOTIFICATION_CHANNEL,
         message: 'Received payload on channel',
         payload: message
@@ -139,7 +139,7 @@ module Gitlab
         # running job
         find_thread_with_lock(jid) do |thread|
           Sidekiq.logger.warn(
-            class: self.class,
+            class: self.class.to_s,
             action: 'cancel',
             message: 'Canceling thread with CancelledError',
             jid: jid,
diff --git a/spec/lib/gitlab/sidekiq_middleware/monitor_spec.rb b/spec/lib/gitlab/sidekiq_middleware/monitor_spec.rb
index 3ca2ddf3cb1..2933d26a387 100644
--- a/spec/lib/gitlab/sidekiq_middleware/monitor_spec.rb
+++ b/spec/lib/gitlab/sidekiq_middleware/monitor_spec.rb
@@ -25,5 +25,17 @@ describe Gitlab::SidekiqMiddleware::Monitor do
 
       expect(result).to eq('value')
     end
+
+    context 'when cancel happens' do
+      subject do
+        monitor.call(worker, job, queue) do
+          raise Gitlab::SidekiqMonitor::CancelledError
+        end
+      end
+
+      it 'does skip this job' do
+        expect { subject }.to raise_error(Sidekiq::JobRetry::Skip)
+      end
+    end
   end
 end
diff --git a/spec/lib/gitlab/sidekiq_monitor_spec.rb b/spec/lib/gitlab/sidekiq_monitor_spec.rb
index 62baa326887..bbd7bf90217 100644
--- a/spec/lib/gitlab/sidekiq_monitor_spec.rb
+++ b/spec/lib/gitlab/sidekiq_monitor_spec.rb
@@ -54,7 +54,7 @@ describe Gitlab::SidekiqMonitor do
       it 'logs start message' do
         expect(Sidekiq.logger).to receive(:info)
           .with(
-            class: described_class,
+            class: described_class.to_s,
             action: 'start',
             message: 'Starting Monitor Daemon')
 
@@ -66,7 +66,7 @@ describe Gitlab::SidekiqMonitor do
       it 'logs stop message' do
         expect(Sidekiq.logger).to receive(:warn)
           .with(
-            class: described_class,
+            class: described_class.to_s,
             action: 'stop',
             message: 'Stopping Monitor Daemon')
 
@@ -78,7 +78,7 @@ describe Gitlab::SidekiqMonitor do
       it 'logs StandardError message' do
         expect(Sidekiq.logger).to receive(:warn)
           .with(
-            class: described_class,
+            class: described_class.to_s,
             action: 'exception',
             message: 'My Exception')
 
@@ -91,7 +91,7 @@ describe Gitlab::SidekiqMonitor do
       it 'logs and raises Exception message' do
         expect(Sidekiq.logger).to receive(:warn)
           .with(
-            class: described_class,
+            class: described_class.to_s,
             action: 'exception',
             message: 'My Exception')
 
@@ -131,13 +131,13 @@ describe Gitlab::SidekiqMonitor do
 
         expect(Sidekiq.logger).to receive(:info)
           .with(
-            class: described_class,
+            class: described_class.to_s,
             action: 'start',
             message: 'Starting Monitor Daemon')
 
         expect(Sidekiq.logger).to receive(:info)
           .with(
-            class: described_class,
+            class: described_class.to_s,
             channel: described_class::NOTIFICATION_CHANNEL,
             message: 'Received payload on channel',
             payload: payload
@@ -215,7 +215,7 @@ describe Gitlab::SidekiqMonitor do
         it 'does log cancellation message' do
           expect(Sidekiq.logger).to receive(:warn)
             .with(
-              class: described_class,
+              class: described_class.to_s,
               action: 'cancel',
               message: 'Canceling thread with CancelledError',
               jid: 'my-jid',