PERF: Introduce absolute limit of digests per 30 minutes (#10845)

PERF: Introduce absolute limit of digests per 30 minutes (#10845)

To avoid blocking the sidekiq queue a limit of 10,000 digests per 30 minutes is introduced.

This acts as a safety measure that makes sure we don’t keep pouring oil on a fire.

On multisites it is recommended to set the number way lower so sites do not dominate the backlog. A reasonable default for multisites may be 100-500.

This can be controlled with the environment var

DISCOURSE_MAX_DIGESTS_ENQUEUED_PER_30_MINS_PER_SITE

diff --git a/app/jobs/scheduled/enqueue_digest_emails.rb b/app/jobs/scheduled/enqueue_digest_emails.rb
index b126dc1..d0fba28 100644
--- a/app/jobs/scheduled/enqueue_digest_emails.rb
+++ b/app/jobs/scheduled/enqueue_digest_emails.rb
@@ -7,7 +7,13 @@ module Jobs
 
     def execute(args)
       return if SiteSetting.disable_digest_emails? || SiteSetting.private_email?
-      target_user_ids.each do |user_id|
+      users = target_user_ids
+
+      if users.length > GlobalSetting.max_digests_enqueued_per_30_mins_per_site
+        users = users.shuffle[0...GlobalSetting.max_digests_enqueued_per_30_mins_per_site]
+      end
+
+      users.each do |user_id|
         ::Jobs.enqueue(:user_email, type: :digest, user_id: user_id)
       end
     end
diff --git a/config/discourse_defaults.conf b/config/discourse_defaults.conf
index c725035..66fe172 100644
--- a/config/discourse_defaults.conf
+++ b/config/discourse_defaults.conf
@@ -309,3 +309,8 @@ allowed_theme_repos =
 # We want this off by default so the process is not started when it does not
 # need to be (e.g. development, test, certain hosting tiers)
 enable_email_sync_demon = false
+
+# we never want to queue more than 10000 digests per 30 minute block
+# this can easily lead to blocking sidekiq
+# on multisites we recommend a far lower number
+max_digests_enqueued_per_30_mins_per_site = 10000
diff --git a/spec/jobs/enqueue_digest_emails_spec.rb b/spec/jobs/enqueue_digest_emails_spec.rb
index fa3fea9..97ab4e3 100644
--- a/spec/jobs/enqueue_digest_emails_spec.rb
+++ b/spec/jobs/enqueue_digest_emails_spec.rb
@@ -121,6 +121,20 @@ describe Jobs::EnqueueDigestEmails do
 
     let(:user) { Fabricate(:user) }
 
+    it "limits jobs enqueued per max_digests_enqueued_per_30_mins_per_site" do
+      Fabricate(:user, last_seen_at: 2.months.ago, last_emailed_at: 2.months.ago)
+      Fabricate(:user, last_seen_at: 2.months.ago, last_emailed_at: 2.months.ago)
+
+      global_setting :max_digests_enqueued_per_30_mins_per_site, 1
+
+      # I don't love fakes, but no point sending this fake email
+      Sidekiq::Testing.fake! do
+        expect do
+          Jobs::EnqueueDigestEmails.new.execute(nil)
+        end.to change(Jobs::UserEmail.jobs, :size).by (1)
+      end
+    end
+
     context "digest emails are enabled" do
       before do
         Jobs::EnqueueDigestEmails.any_instance.expects(:target_user_ids).returns([user.id])

GitHub sha: 120fa8ad

1 Like

This commit appears in #10845 which was approved by jjaffeux. It was merged by SamSaffron.