FIX: Handle SMTPServerBusy for group smtp email (#13632)

FIX: Handle SMTPServerBusy for group smtp email (#13632)

Use the sidekiq_retry_in code from Jobs::UserEmail in group SMTP. Also we don’t need to keep seconds_to_delay – sidekiq uses the default delay calculation if you return 0 or nil from the block. See sidekiq/job_retry.rb at 3330df0ee37cfd3e0cd3ef01e3e66b584b99d488 · mperham/sidekiq · GitHub for sidekiq default retry delay logic.

I experimented with extracting this into a concern or a module, but sidekiq_retry_in is quite magic and it would not allow me to abstract away into a module that calls some method specificall in the child job class.

I would love to write tests for this, but it does not seem possible (not sure if its because of our test setup) to write tests that test sidekiq’s retry capability, and I am not sure if we should be anyway. Initial addition to UserEmail did not test this functionality FIX: retry sending an email in 1 hour when SMTP server is busy · discourse/discourse@d224966 · GitHub

diff --git a/app/jobs/regular/group_smtp_email.rb b/app/jobs/regular/group_smtp_email.rb
index 66eadb9..ce113e5 100644
--- a/app/jobs/regular/group_smtp_email.rb
+++ b/app/jobs/regular/group_smtp_email.rb
@@ -8,6 +8,19 @@ module Jobs
 
     sidekiq_options queue: 'critical'
 
+    sidekiq_retry_in do |count, exception|
+      # retry in an hour when SMTP server is busy
+      # or use default sidekiq retry formula. returning
+      # nil/0 will trigger the default sidekiq
+      # retry formula
+      #
+      # See https://github.com/mperham/sidekiq/blob/3330df0ee37cfd3e0cd3ef01e3e66b584b99d488/lib/sidekiq/job_retry.rb#L216-L234
+      case exception.wrapped
+      when Net::SMTPServerBusy
+        return 1.hour + (rand(30) * (count + 1))
+      end
+    end
+
     def execute(args)
       return if quit_email_early?
 
diff --git a/app/jobs/regular/notify_mailing_list_subscribers.rb b/app/jobs/regular/notify_mailing_list_subscribers.rb
index 3f71efb..861dc27 100644
--- a/app/jobs/regular/notify_mailing_list_subscribers.rb
+++ b/app/jobs/regular/notify_mailing_list_subscribers.rb
@@ -12,11 +12,13 @@ module Jobs
     sidekiq_options retry: RETRY_TIMES.size
 
     sidekiq_retry_in do |count, exception|
+      # returning nil/0 will trigger the default sidekiq
+      # retry formula
+      #
+      # See https://github.com/mperham/sidekiq/blob/3330df0ee37cfd3e0cd3ef01e3e66b584b99d488/lib/sidekiq/job_retry.rb#L216-L234
       case exception.wrapped
       when SocketError
-        RETRY_TIMES[count]
-      else
-        ::Jobs::UserEmail.seconds_to_delay(count)
+        return RETRY_TIMES[count]
       end
     end
 
diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb
index df53f47..d63d17e 100644
--- a/app/jobs/regular/user_email.rb
+++ b/app/jobs/regular/user_email.rb
@@ -8,6 +8,19 @@ module Jobs
 
     sidekiq_options queue: 'low'
 
+    sidekiq_retry_in do |count, exception|
+      # retry in an hour when SMTP server is busy
+      # or use default sidekiq retry formula. returning
+      # nil/0 will trigger the default sidekiq
+      # retry formula
+      #
+      # See https://github.com/mperham/sidekiq/blob/3330df0ee37cfd3e0cd3ef01e3e66b584b99d488/lib/sidekiq/job_retry.rb#L216-L234
+      case exception.wrapped
+      when Net::SMTPServerBusy
+        return 1.hour + (rand(30) * (count + 1))
+      end
+    end
+
     # Can be overridden by subclass, for example critical email
     # should always consider being sent
     def quit_email_early?
@@ -206,22 +219,6 @@ module Jobs
       [message, nil]
     end
 
-    sidekiq_retry_in do |count, exception|
-      # retry in an hour when SMTP server is busy
-      # or use default sidekiq retry formula
-      case exception.wrapped
-      when Net::SMTPServerBusy
-        1.hour + (rand(30) * (count + 1))
-      else
-        ::Jobs::UserEmail.seconds_to_delay(count)
-      end
-    end
-
-    # extracted from sidekiq
-    def self.seconds_to_delay(count)
-      (count**4) + 15 + (rand(30) * (count + 1))
-    end
-
     private
 
     def skip_message(reason)

GitHub sha: b3d3ad250beb8642ec8a9be5c69007c498544f4f

This commit appears in #13632 which was approved by lis2. It was merged by martin.