From 3dbbb940dee5864b05484d122ae3da69694b43ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= Date: Wed, 5 Mar 2025 17:17:30 +0100 Subject: [PATCH] DEV: Upgrade Sidekiq to v7.3.9 --- Gemfile.lock | 15 +++++++---- app/jobs/base.rb | 2 +- config/initializers/100-sidekiq.rb | 7 ++--- .../20250227142351_migrate_sidekiq_jobs.rb | 3 +-- lib/backup_restore/system_interface.rb | 2 +- lib/discourse.rb | 15 +++++------ lib/freedom_patches/sidekiq.rb | 19 ++++++++------ lib/sidekiq_logster_reporter.rb | 2 +- lib/sidekiq_long_running_job_logger.rb | 7 +++-- lib/sidekiq_migration.rb | 3 +-- .../backup_restore/system_interface_spec.rb | 12 +++------ spec/lib/discourse_spec.rb | 6 ++--- .../sidekiq_long_running_job_logger_spec.rb | 26 ++++++++++--------- spec/rails_helper.rb | 2 +- 14 files changed, 62 insertions(+), 59 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 64f86f6dc88..1f43880cadd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -428,6 +428,8 @@ GEM psych (>= 4.0.0) redcarpet (3.6.1) redis (4.8.1) + redis-client (0.23.2) + connection_pool redis-namespace (1.11.0) redis (>= 4) regexp_parser (2.10.0) @@ -558,10 +560,12 @@ GEM websocket (~> 1.0) shoulda-matchers (6.4.0) activesupport (>= 5.2.0) - sidekiq (6.5.12) - connection_pool (>= 2.2.5, < 3) - rack (~> 2.0) - redis (>= 4.5.0, < 5) + sidekiq (7.3.9) + base64 + connection_pool (>= 2.3.0) + logger + rack (>= 2.2.4) + redis-client (>= 0.22.2) simplecov (0.22.0) docile (~> 1.1) simplecov-html (~> 0.11) @@ -999,6 +1003,7 @@ CHECKSUMS rdoc (6.12.0) sha256=7d6f706e070bffa5d18a448f24076cbfb34923a99c1eab842aa18e6ca69f56e0 redcarpet (3.6.1) sha256=d444910e6aa55480c6bcdc0cdb057626e8a32c054c29e793fa642ba2f155f445 redis (4.8.1) sha256=387ee086694fffc9632aaeb1efe4a7b1627ca783bf373320346a8a20cd93333a + redis-client (0.23.2) sha256=e33bab6682c8155cfef95e6dd296936bb9c2981a89fb578ace27a076fa2836fa redis-namespace (1.11.0) sha256=e91a1aa2b2d888b6dea1d4ab8d39e1ae6fac3426161feb9d91dd5cca598a2239 regexp_parser (2.10.0) sha256=cb6f0ddde88772cd64bff1dbbf68df66d376043fe2e66a9ef77fcb1b0c548c61 reline (0.6.0) sha256=57620375dcbe56ec09bac7192bfb7460c716bbf0054dc94345ecaa5438e539d2 @@ -1048,7 +1053,7 @@ CHECKSUMS selenium-devtools (0.133.0) sha256=aba5a5225ac38d235bbc6ebb4e0b8209e973ac7af4226e4304a1417573f73f64 selenium-webdriver (4.29.1) sha256=0a7fe53cc4d2c515adbb89e115c6e786c64e9b98f85939d21071c6e32883a146 shoulda-matchers (6.4.0) sha256=9055bb7f4bb342125fb860809798855c630e05ef5e75837b3168b8e6ee1608b0 - sidekiq (6.5.12) sha256=b4f93b2204c42220d0b526a7b8e0c49b5f9da82c1ce1a05d2baf1e8f744c197f + sidekiq (7.3.9) sha256=1108712e1def89002b28e3545d5ae15d4a57ffd4d2c25d97bb1360988826b5a7 simplecov (0.22.0) sha256=fe2622c7834ff23b98066bb0a854284b2729a569ac659f82621fc22ef36213a5 simplecov-html (0.13.1) sha256=5dab0b7ee612e60e9887ad57693832fdf4695b4c0c859eaea5f95c18791ef10b simplecov_json_formatter (0.1.4) sha256=529418fbe8de1713ac2b2d612aa3daa56d316975d307244399fa4838c601b428 diff --git a/app/jobs/base.rb b/app/jobs/base.rb index a5d4eb2b0fe..e3d539541b2 100644 --- a/app/jobs/base.rb +++ b/app/jobs/base.rb @@ -475,7 +475,7 @@ module Jobs Sidekiq::ScheduledSet.new.select do |scheduled_job| if scheduled_job.klass.to_s == job_class matched = true - job_params = scheduled_job.item["args"][0].with_indifferent_access + job_params = scheduled_job.args[0].with_indifferent_access opts.each do |key, value| if job_params[key] != value matched = false diff --git a/config/initializers/100-sidekiq.rb b/config/initializers/100-sidekiq.rb index fdebafb41e3..6932de68965 100644 --- a/config/initializers/100-sidekiq.rb +++ b/config/initializers/100-sidekiq.rb @@ -66,16 +66,17 @@ else # # Instead, this patch adds a dedicated logger instance and patches # the #add method to forward messages to Rails.logger. - Sidekiq.logger = Logger.new(nil) + Sidekiq.default_configuration.logger = Logger.new(nil) Sidekiq + .default_configuration .logger .define_singleton_method(:add) do |severity, message = nil, progname = nil, &blk| Rails.logger.add(severity, message, progname, &blk) end end -Sidekiq.error_handlers.clear -Sidekiq.error_handlers << SidekiqLogsterReporter.new +Sidekiq.default_configuration.error_handlers.clear +Sidekiq.default_configuration.error_handlers << SidekiqLogsterReporter.new Sidekiq.strict_args! diff --git a/db/post_migrate/20250227142351_migrate_sidekiq_jobs.rb b/db/post_migrate/20250227142351_migrate_sidekiq_jobs.rb index 64806ee419d..a0e6237aa05 100644 --- a/db/post_migrate/20250227142351_migrate_sidekiq_jobs.rb +++ b/db/post_migrate/20250227142351_migrate_sidekiq_jobs.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true -# Delete this migration instead of promoting it (along with the -# `SidekiqMigration` class) +# TODO: Remove this after the Discourse 3.5 release class MigrateSidekiqJobs < ActiveRecord::Migration[7.2] def up SidekiqMigration.call diff --git a/lib/backup_restore/system_interface.rb b/lib/backup_restore/system_interface.rb index 31bd4eb944e..cbe55c3d0e1 100644 --- a/lib/backup_restore/system_interface.rb +++ b/lib/backup_restore/system_interface.rb @@ -115,7 +115,7 @@ module BackupRestore def sidekiq_has_running_jobs? Sidekiq::Workers.new.each do |_, _, work| - args = work&.dig("payload", "args")&.first + args = work.job.args&.first current_site_id = args["current_site_id"] if args.present? return true if current_site_id.blank? || current_site_id == @current_db diff --git a/lib/discourse.rb b/lib/discourse.rb index 440bbbbf87a..04332b4f73a 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -210,7 +210,7 @@ module Discourse return if ex.class == Jobs::HandledExceptionWrapper context ||= {} - parent_logger ||= Sidekiq + parent_logger ||= Sidekiq.default_configuration job = context[:job] @@ -930,10 +930,7 @@ module Discourse Rails.cache.reconnect Discourse.cache.reconnect Logster.store.redis.reconnect - # shuts down all connections in the pool - Sidekiq.redis_pool.shutdown { |conn| conn.disconnect! } - # re-establish - Sidekiq.redis = sidekiq_redis_config + Sidekiq.redis_pool.reload(&:close) # in case v8 was initialized we want to make sure it is nil PrettyText.reset_context @@ -1048,9 +1045,11 @@ module Discourse SIDEKIQ_NAMESPACE = "sidekiq" def self.sidekiq_redis_config(old: false) - redis_config = GlobalSetting.redis_config.dup - return redis_config.merge(namespace: SIDEKIQ_NAMESPACE) if old - redis_config.merge(db: redis_config[:db].to_i + 1) + GlobalSetting + .redis_config + .dup + .except(:connector, :replica_host, :replica_port) + .tap { |config| config.merge!(db: config[:db].to_i + 1) unless old } end def self.static_doc_topic_ids diff --git a/lib/freedom_patches/sidekiq.rb b/lib/freedom_patches/sidekiq.rb index 656759dcf8d..6d91a40009a 100644 --- a/lib/freedom_patches/sidekiq.rb +++ b/lib/freedom_patches/sidekiq.rb @@ -1,14 +1,17 @@ # frozen_string_literal: true -# TODO (2025-03-03): delete this once the migration is propagated everywhere -# (in about 6 months or so) +# TODO: Remove this after the Discourse 3.5 release module Sidekiq - def self.redis_pool - @redis ||= RedisConnection.create - Thread.current[:sidekiq_via_pool] || @redis - end - def self.old_pool - @old_pool ||= RedisConnection.create(Discourse.sidekiq_redis_config(old: true)) + @old_pool ||= + begin + ConnectionPool.new do + Redis::Namespace.new( + Discourse::SIDEKIQ_NAMESPACE, + redis: + Sidekiq::RedisClientAdapter.new(Discourse.sidekiq_redis_config(old: true)).new_client, + ) + end + end end end diff --git a/lib/sidekiq_logster_reporter.rb b/lib/sidekiq_logster_reporter.rb index d7be5939665..72a82b1a461 100644 --- a/lib/sidekiq_logster_reporter.rb +++ b/lib/sidekiq_logster_reporter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class SidekiqLogsterReporter - def call(ex, context = {}) + def call(ex, context = {}, _config = nil) return if Jobs::HandledExceptionWrapper === ex Discourse.reset_active_record_cache_if_needed(ex) diff --git a/lib/sidekiq_long_running_job_logger.rb b/lib/sidekiq_long_running_job_logger.rb index e6152d0c137..9d3b1081013 100644 --- a/lib/sidekiq_long_running_job_logger.rb +++ b/lib/sidekiq_long_running_job_logger.rb @@ -28,19 +28,18 @@ class SidekiqLongRunningJobLogger Sidekiq::Workers.new.each do |process_id, thread_id, work| next unless process_id.start_with?(hostname) - if Time.at(work["run_at"]).to_i >= - (Time.now - (60 * @stuck_sidekiq_job_minutes)).to_i + if Time.at(work.run_at).to_i >= (Time.now - (60 * @stuck_sidekiq_job_minutes)).to_i next end - jid = work.dig("payload", "jid") + jid = work.job.jid current_long_running_jobs << jid next if @seen_long_running_jobs&.include?(jid) if thread = Thread.list.find { |t| t["sidekiq_tid"] == thread_id } Rails.logger.warn(<<~MSG) - Sidekiq job `#{work.dig("payload", "class")}` has been running for more than #{@stuck_sidekiq_job_minutes} minutes + Sidekiq job `#{work.job.klass}` has been running for more than #{@stuck_sidekiq_job_minutes} minutes #{thread.backtrace.join("\n")} MSG end diff --git a/lib/sidekiq_migration.rb b/lib/sidekiq_migration.rb index e82195f8e13..12df82fd273 100644 --- a/lib/sidekiq_migration.rb +++ b/lib/sidekiq_migration.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true -# TODO (2025-03-03): delete this once the migration is propagated everywhere -# (in about 6 months or so) +# TODO: Remove this after the Discourse 3.5 release class SidekiqMigration delegate :old_pool, to: :Sidekiq diff --git a/spec/lib/backup_restore/system_interface_spec.rb b/spec/lib/backup_restore/system_interface_spec.rb index 191e220cb63..0f7caee3586 100644 --- a/spec/lib/backup_restore/system_interface_spec.rb +++ b/spec/lib/backup_restore/system_interface_spec.rb @@ -105,11 +105,7 @@ RSpec.describe BackupRestore::SystemInterface do end context "with Sidekiq workers" do - after { flush_sidekiq_redis_namespace } - - def flush_sidekiq_redis_namespace - Sidekiq.redis { |redis| redis.scan_each { |key| redis.del(key) } } - end + after { Sidekiq.redis(&:flushdb) } def create_workers(site_id: nil, all_sites: false) payload = @@ -132,8 +128,8 @@ RSpec.describe BackupRestore::SystemInterface do key = "#{hostname}:#{pid}" process = { pid: pid, hostname: hostname } - conn.sadd?("processes", key) - conn.hmset(key, "info", Sidekiq.dump_json(process)) + conn.sadd("processes", key) + conn.hset(key, "info", Sidekiq.dump_json(process)) data = Sidekiq.dump_json( @@ -141,7 +137,7 @@ RSpec.describe BackupRestore::SystemInterface do run_at: Time.now.to_i, payload: Sidekiq.dump_json(payload), ) - conn.hmset("#{key}:work", "444", data) + conn.hset("#{key}:work", "444", data) end end diff --git a/spec/lib/discourse_spec.rb b/spec/lib/discourse_spec.rb index 07408118aad..9821dfc38a4 100644 --- a/spec/lib/discourse_spec.rb +++ b/spec/lib/discourse_spec.rb @@ -362,7 +362,7 @@ RSpec.describe Discourse do class TempSidekiqLogger attr_accessor :exception, :context - def call(ex, ctx) + def call(ex, ctx, _config) self.exception = ex self.context = ctx end @@ -370,9 +370,9 @@ RSpec.describe Discourse do let!(:logger) { TempSidekiqLogger.new } - before { Sidekiq.error_handlers << logger } + before { Sidekiq.default_configuration.error_handlers << logger } - after { Sidekiq.error_handlers.delete(logger) } + after { Sidekiq.default_configuration.error_handlers.delete(logger) } describe "#job_exception_stats" do class FakeTestError < StandardError diff --git a/spec/lib/sidekiq_long_running_job_logger_spec.rb b/spec/lib/sidekiq_long_running_job_logger_spec.rb index 8964bd9cd93..8567b34d44e 100644 --- a/spec/lib/sidekiq_long_running_job_logger_spec.rb +++ b/spec/lib/sidekiq_long_running_job_logger_spec.rb @@ -20,24 +20,26 @@ RSpec.describe SidekiqLongRunningJobLogger do [ "#{hostname}:1234", "some_sidekiq_id", - { - "run_at" => (Time.now - (60 * (stuck_sidekiq_job_minutes + 1))).to_i, - "payload" => { - "jid" => "job_1", - "class" => "TestWorker", + Sidekiq::Work.new( + "#{hostname}:1234", + "some_sidekiq_id", + { + "run_at" => (Time.now - (60 * (stuck_sidekiq_job_minutes + 1))).to_i, + "payload" => Sidekiq.dump_json({ "jid" => "job_1", "class" => "TestWorker" }), }, - }, + ), ], [ "#{hostname}:1234", "some_other_sidekiq_id", - { - "run_at" => Time.now.to_i, - "payload" => { - "jid" => "job_2", - "class" => "AnotherWorker", + Sidekiq::Work.new( + "#{hostname}:1234", + "some_other_sidekiq_id", + { + "run_at" => Time.now.to_i, + "payload" => Sidekiq.dump_json({ "jid" => "job_2", "class" => "AnotherWorker" }), }, - }, + ), ], ], ) diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 78afa43cd2b..f46721ae88a 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -288,7 +288,7 @@ RSpec.configure do |config| raise "There are pending migrations, run RAILS_ENV=test bin/rake db:migrate" end - Sidekiq.error_handlers.clear + Sidekiq.default_configuration.error_handlers.clear # Ugly, but needed until we have a user creator User.skip_callback(:create, :after, :ensure_in_trust_level_group)