From 3ae904b992eee2d95cb4b44a333e3cff4d97f126 Mon Sep 17 00:00:00 2001 From: Rosa Gutierrez Date: Sun, 19 Jul 2026 14:43:14 +0200 Subject: [PATCH] Refactor errors and warnings when parsing configuration We had two different warnings being surfaced in totally different ways, and at the same time, warnings being totally independent and different from errors. Unify everything and make them consistent. This is a follow-up to #661, #753 and #745 --- lib/solid_queue/configuration.rb | 71 ++++++++++++++++---------------- lib/solid_queue/supervisor.rb | 2 +- lib/solid_queue/tasks.rb | 3 +- test/unit/cli_test.rb | 2 +- test/unit/configuration_test.rb | 44 ++++++++++++-------- test/unit/rake_tasks_test.rb | 2 +- 6 files changed, 67 insertions(+), 57 deletions(-) diff --git a/lib/solid_queue/configuration.rb b/lib/solid_queue/configuration.rb index b547d12c..d04a0aaa 100644 --- a/lib/solid_queue/configuration.rb +++ b/lib/solid_queue/configuration.rb @@ -3,9 +3,12 @@ module SolidQueue class Configuration include ActiveModel::Model + include ActiveModel::Validations::Callbacks - validate :ensure_configured_processes - validate :ensure_valid_recurring_tasks + validate :ensure_configured_processes, :ensure_valid_recurring_tasks + validate :warn_about_incorrectly_sized_thread_pool, :warn_about_missing_config_files + + before_validation { warnings.clear } class Process < Struct.new(:kind, :attributes) def instantiate @@ -46,43 +49,30 @@ def configured_processes end end - def error_messages - if configured_processes.none? - "No workers or processed configured. Exiting..." - else - error_messages = invalid_tasks.map do |task| - all_messages = task.errors.full_messages.map { |msg| "\t#{msg}" }.join("\n") - "#{task.key}:\n#{all_messages}" - end - .join("\n") - - "Invalid processes configured:\n#{error_messages}" - end - end - def mode - @options[:mode].to_s.inquiry + options[:mode].to_s.inquiry end def standalone? - mode.fork? || @options[:standalone] + mode.fork? || options[:standalone] end def warnings - @warnings ||= [ undersized_thread_pool_message ].compact + @warnings ||= ActiveModel::Errors.new(self) end def check - warnings.each { |warning| $stdout.puts "Warning: #{warning}" } - if valid? + warnings.full_messages.each { |warning| $stderr.puts warning } $stdout.puts "Solid Queue configuration is valid." + true else - $stderr.puts "Invalid Solid Queue configuration:" - errors.full_messages.each do |message| + $stderr.puts "Solid Queue configuration is invalid:" + (warnings.full_messages + errors.full_messages).each do |message| message.each_line { |line| $stderr.puts " #{line.chomp}" } end + false end end @@ -90,18 +80,6 @@ def check private attr_reader :options - def undersized_thread_pool_message - db_pool_size = SolidQueue::Record.connection_pool&.size - - if db_pool_size && db_pool_size < estimated_number_of_threads - "Solid Queue is configured to use #{estimated_number_of_threads} threads but the " \ - "database connection pool is #{db_pool_size}. Increase it in `config/database.yml`" - end - rescue ActiveRecord::ActiveRecordError - # No usable database connection. Skip the pool-size warning in that case. - nil - end - def ensure_configured_processes unless configured_processes.any? errors.add(:base, "No processes configured") @@ -118,6 +96,28 @@ def ensure_valid_recurring_tasks end end + def warn_about_incorrectly_sized_thread_pool + db_pool_size = SolidQueue::Record.connection_pool&.size + + if db_pool_size && db_pool_size < estimated_number_of_threads + warnings.add(:base, "Warning: Solid Queue is configured to use #{estimated_number_of_threads} threads but the " \ + "database connection pool is #{db_pool_size}. Increase it in `config/database.yml`") + end + rescue ActiveRecord::ActiveRecordError + # No usable database connection. Skip the pool-size warning in that case. + end + + def warn_about_missing_config_files + files = [ options[:config_file] ] + files << options[:recurring_schedule_file] unless skip_recurring_tasks? + + files.compact.each do |file| + unless Pathname.new(file).exist? + warnings.add(:base, "Warning: provided configuration file '#{file}' does not exist. Falling back to default configuration.") + end + end + end + def default_options { mode: ENV["SOLID_QUEUE_SUPERVISOR_MODE"] || :fork, @@ -244,7 +244,6 @@ def load_config_from_file(file) if file.exist? ActiveSupport::ConfigurationFile.parse(file).deep_symbolize_keys else - puts "[solid_queue] WARNING: Provided configuration file '#{file}' does not exist. Falling back to default configuration." {} end end diff --git a/lib/solid_queue/supervisor.rb b/lib/solid_queue/supervisor.rb index ffd4d3bd..17b90c54 100644 --- a/lib/solid_queue/supervisor.rb +++ b/lib/solid_queue/supervisor.rb @@ -13,7 +13,7 @@ def start(**options) configuration = Configuration.new(**options) if configuration.valid? - configuration.warnings.each { |warning| SolidQueue.logger.warn(warning) } + configuration.warnings.full_messages.each { |warning| SolidQueue.logger.warn(warning) } klass = configuration.mode.fork? ? ForkSupervisor : AsyncSupervisor klass.new(configuration).tap(&:start) diff --git a/lib/solid_queue/tasks.rb b/lib/solid_queue/tasks.rb index 026cb42e..70422562 100644 --- a/lib/solid_queue/tasks.rb +++ b/lib/solid_queue/tasks.rb @@ -11,6 +11,7 @@ desc "validate the Solid Queue configuration for the current Rails env without starting any process" task check: :environment do - exit 1 unless SolidQueue::Configuration.new.check + configuration = SolidQueue::Configuration.new + exit 1 unless configuration.check end end diff --git a/test/unit/cli_test.rb b/test/unit/cli_test.rb index 12e97dcf..f3d5415a 100644 --- a/test/unit/cli_test.rb +++ b/test/unit/cli_test.rb @@ -51,7 +51,7 @@ class CliTest < ActiveSupport::TestCase ) assert_equal 1, exit_status - assert_match "Invalid Solid Queue configuration", err + assert_match "Solid Queue configuration is invalid", err assert_match "periodic_invalid_class", err assert_match "periodic_incorrect_schedule", err assert_empty out diff --git a/test/unit/configuration_test.rb b/test/unit/configuration_test.rb index aeec5e5b..7bb7f70a 100644 --- a/test/unit/configuration_test.rb +++ b/test/unit/configuration_test.rb @@ -27,10 +27,11 @@ class ConfigurationTest < ActiveSupport::TestCase end test "warns if provided configuration file does not exist" do - assert_output "[solid_queue] WARNING: Provided configuration file '/path/to/nowhere.yml' does not exist. Falling back to default configuration.\n" do - configuration = SolidQueue::Configuration.new(config_file: Pathname.new("/path/to/nowhere.yml")) - assert configuration.valid? - end + configuration = SolidQueue::Configuration.new(config_file: Pathname.new("/path/to/nowhere.yml")) + + assert configuration.valid? + assert_includes configuration.warnings.full_messages, + "Warning: provided configuration file '/path/to/nowhere.yml' does not exist. Falling back to default configuration." end test "read configuration from default file" do @@ -145,20 +146,20 @@ class ConfigurationTest < ActiveSupport::TestCase assert error.include?("periodic_invalid_class: Class name doesn't correspond to an existing class") assert error.include?("periodic_incorrect_schedule: Schedule is not a supported recurring schedule") - assert_output(/Provided configuration file '[^']+' does not exist\./) do - assert SolidQueue::Configuration.new(recurring_schedule_file: config_file_path(:empty_recurring)).valid? - end + configuration = SolidQueue::Configuration.new(recurring_schedule_file: config_file_path(:empty_recurring)) + assert configuration.valid? + assert_match(/provided configuration file '[^']+' does not exist\./, configuration.warnings.full_messages.join) + assert SolidQueue::Configuration.new(skip_recurring: true).valid? configuration = SolidQueue::Configuration.new(recurring_schedule_file: config_file_path(:recurring_with_production_only)) assert configuration.valid? assert_processes configuration, :scheduler, 0 - assert_output(/Provided configuration file '[^']+' does not exist\./) do - configuration = SolidQueue::Configuration.new(recurring_schedule_file: config_file_path(:recurring_with_empty)) - assert configuration.valid? - assert_processes configuration, :scheduler, 0 - end + configuration = SolidQueue::Configuration.new(recurring_schedule_file: config_file_path(:recurring_with_empty)) + assert configuration.valid? + assert_processes configuration, :scheduler, 0 + assert_match(/provided configuration file '[^']+' does not exist\./, configuration.warnings.full_messages.join) # No processes configuration = SolidQueue::Configuration.new(skip_recurring: true, dispatchers: [], workers: []) @@ -175,15 +176,24 @@ class ConfigurationTest < ActiveSupport::TestCase assert configuration.valid? assert_equal 1, configuration.warnings.size - assert_match /Solid Queue is configured to use \d+ threads but the database connection pool is \d+\. Increase it in `config\/database.yml`/, configuration.warnings.first + assert_match /Solid Queue is configured to use \d+ threads but the database connection pool is \d+\. Increase it in `config\/database.yml`/, configuration.warnings.full_messages.first end test "has no warnings when the database connection pool is large enough" do configuration = SolidQueue::Configuration.new(workers: [ { queues: "background", threads: 1, polling_interval: 10 } ], skip_recurring: true) + assert configuration.valid? assert_empty configuration.warnings end + test "does not duplicate warnings when validated more than once" do + configuration = SolidQueue::Configuration.new(workers: [ { queues: "background", threads: 50, polling_interval: 10 } ], skip_recurring: true) + + 3.times { configuration.valid? } + + assert_equal 1, configuration.warnings.size + end + test "check prints a success message and returns true for a valid configuration" do out, err = capture_io do assert SolidQueue::Configuration.new(skip_recurring: true).check @@ -193,13 +203,13 @@ class ConfigurationTest < ActiveSupport::TestCase assert_empty err end - test "check prints warnings to stdout on the valid path" do - out, _err = capture_io do + test "check prints warnings to stderr on the valid path" do + out, err = capture_io do assert SolidQueue::Configuration.new(workers: [ { queues: "background", threads: 50, polling_interval: 10 } ], skip_recurring: true).check end assert_match "Solid Queue configuration is valid.", out - assert_match /Warning: Solid Queue is configured to use \d+ threads but the database connection pool is \d+/, out + assert_match /Solid Queue is configured to use \d+ threads but the database connection pool is \d+/, err end test "check prints errors to stderr and returns false for an invalid configuration" do @@ -207,7 +217,7 @@ class ConfigurationTest < ActiveSupport::TestCase assert_not SolidQueue::Configuration.new(recurring_schedule_file: config_file_path(:recurring_with_invalid)).check end - assert_match "Invalid Solid Queue configuration:", err + assert_match "Solid Queue configuration is invalid:", err assert_match "periodic_invalid_class", err end diff --git a/test/unit/rake_tasks_test.rb b/test/unit/rake_tasks_test.rb index 8b2de143..717bd980 100644 --- a/test/unit/rake_tasks_test.rb +++ b/test/unit/rake_tasks_test.rb @@ -44,7 +44,7 @@ class RakeTasksTest < ActiveSupport::TestCase assert_equal 1, status assert_empty out - assert_match "Invalid Solid Queue configuration:", err + assert_match "Solid Queue configuration is invalid:", err assert_match "broken", err end end