Fix report_after_job_retries's decision logic (#1704)

* Fix report_after_job_retries's decision logic

* Update changelog

Author: st0012

CHANGELOG.md

@@ -3,6 +3,8 @@
 - Respect port info provided in user's DSN [#1702](https://github.com/getsentry/sentry-ruby/pull/1702)
   - Fixes [#1699](https://github.com/getsentry/sentry-ruby/issues/1699)
 - Capture transaction tags [#1701](https://github.com/getsentry/sentry-ruby/pull/1701)
+- Fix `report_after_job_retries`'s decision logic [#1704](https://github.com/getsentry/sentry-ruby/pull/1704)
+  - Fixes [#1698](https://github.com/getsentry/sentry-ruby/issues/1698)
 
 ## 5.0.1
 

sentry-sidekiq/lib/sentry/sidekiq/error_handler.rb

@@ -11,11 +11,9 @@ def call(ex, context)
         scope = Sentry.get_current_scope
         scope.set_transaction_name(context_filter.transaction_name) unless scope.transaction_name
 
-        retry_option = context.dig(:job, "retry")
-
-        if Sentry.configuration.sidekiq.report_after_job_retries && retry_option.is_a?(Integer) && retry_option.positive?
+        if Sentry.configuration.sidekiq.report_after_job_retries && retryable?(context)
           retry_count = context.dig(:job, "retry_count")
-          if retry_count.nil? || retry_count < retry_option - 1
+          if retry_count.nil? || retry_count < retry_limit(context) - 1
             return
           end
         end
@@ -26,6 +24,27 @@ def call(ex, context)
           hint: { background: false }
         )
       end
+
+      private
+
+      def retryable?(context)
+        retry_option = context.dig(:job, "retry")
+        # when `retry` is not specified, it's default is `true` and it means 25 retries.
+        retry_option == true || (retry_option.is_a?(Integer) && retry_option.positive?)
+      end
+
+      def retry_limit(context)
+        limit = context.dig(:job, "retry")
+
+        case limit
+        when Integer
+          limit
+        when TrueClass
+          ::Sidekiq.options[:max_retries] || 25
+        else
+          0
+        end
+      end
     end
   end
 end

sentry-sidekiq/spec/sentry/sidekiq_spec.rb

@@ -94,31 +94,90 @@
       Sentry.configuration.sidekiq.report_after_job_retries = true
     end
 
-    it "doesn't report the error until retries are exhuasted" do
-      execute_worker(processor, RetryWorker)
-
-      expect(transport.events.count).to eq(0)
-
-      expect(retry_set.count).to eq(1)
-
+    def retry_last_failed_job
       retry_set.first.add_to_queue
       job = queue.first
       work = Sidekiq::BasicFetch::UnitOfWork.new('queue:default', job.value)
       process_work(processor, work)
-      expect(transport.events.count).to eq(1)
     end
 
-    it "doesn't affect no-retry jobs" do
-      execute_worker(processor, SadWorker)
+    context "when retry: is specified" do
+      it "doesn't report the error until retries are exhuasted" do
+        worker = Class.new(SadWorker)
+        worker.sidekiq_options retry: 5
+        execute_worker(processor, worker)
+        expect(transport.events.count).to eq(0)
+        expect(retry_set.count).to eq(1)
+
+        4.times do |i|
+          retry_last_failed_job
+          expect(transport.events.count).to eq(0)
+        end
+
+        retry_last_failed_job
+        expect(transport.events.count).to eq(1)
+      end
+    end
 
-      expect(transport.events.count).to eq(1)
-      expect(retry_set.count).to eq(1)
+    context "when the job has 0 retries" do
+      it "reports on the first failure" do
+        worker = Class.new(SadWorker)
+        worker.sidekiq_options retry: 0
+
+        execute_worker(processor, worker)
+
+        expect(transport.events.count).to eq(1)
+      end
     end
 
-    it "doesn't affect jobs with zero retries" do
-      execute_worker(processor, ZeroRetryWorker)
+    context "when the job has retry: false" do
+      it "reports on the first failure" do
+        worker = Class.new(SadWorker)
+        worker.sidekiq_options retry: false
 
-      expect(transport.events.count).to eq(1)
+        execute_worker(processor, worker)
+
+        expect(transport.events.count).to eq(1)
+      end
+    end
+
+    context "when retry is not specified on the worker" do
+      before do
+        # this is required for Sidekiq to assign default options to the worker
+        SadWorker.sidekiq_options
+      end
+
+      it "reports on the 25th retry" do
+        execute_worker(processor, SadWorker)
+        expect(transport.events.count).to eq(0)
+        expect(retry_set.count).to eq(1)
+
+        24.times do |i|
+          retry_last_failed_job
+          expect(transport.events.count).to eq(0)
+        end
+
+        retry_last_failed_job
+        expect(transport.events.count).to eq(1)
+      end
+
+      context "when Sidekiq.options[:max_retries] is set" do
+        it "respects the set limit" do
+          Sidekiq.options[:max_retries] = 5
+
+          execute_worker(processor, SadWorker)
+          expect(transport.events.count).to eq(0)
+          expect(retry_set.count).to eq(1)
+
+          4.times do |i|
+            retry_last_failed_job
+            expect(transport.events.count).to eq(0)
+          end
+
+          retry_last_failed_job
+          expect(transport.events.count).to eq(1)
+        end
+      end
     end
   end
 

sentry-sidekiq/spec/spec_helper.rb

@@ -133,26 +133,6 @@ def perform
   end
 end
 
-class RetryWorker
-  include Sidekiq::Worker
-
-  sidekiq_options retry: 1
-
-  def perform
-    1/0
-  end
-end
-
-class ZeroRetryWorker
-  include Sidekiq::Worker
-
-  sidekiq_options retry: 0
-
-  def perform
-    1/0
-  end
-end
-
 class TagsWorker
   include Sidekiq::Worker