From 120fa8ad2fbe988694c017bf71bb998df37f2083 Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 7 Oct 2020 17:30:15 +1100 Subject: [PATCH] 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 --- app/jobs/scheduled/enqueue_digest_emails.rb | 8 +++++++- config/discourse_defaults.conf | 5 +++++ spec/jobs/enqueue_digest_emails_spec.rb | 14 ++++++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/app/jobs/scheduled/enqueue_digest_emails.rb b/app/jobs/scheduled/enqueue_digest_emails.rb index b126dc1553..d0fba281b9 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 c7250354b4..66fe1727cc 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 fa3fea9f89..97ab4e3854 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])