Skip to content

Error When an Invalid Recurring Task is Configured #427

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Error when recurring tasks are invalid
Resolves #414
  • Loading branch information
jherdman committed Dec 3, 2024
commit dda0e4841efbef237f172e10de67e11539ee2615
10 changes: 9 additions & 1 deletion lib/solid_queue/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@ def configured_processes
end
end

def valid?
skip_recurring_tasks? || recurring_tasks.none? || recurring_tasks.all?(&:valid?)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this is probably not exhaustive enough, but I'm unsure of other cases to expand upon. This is probably sufficient for the scope of this PR though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we include perhaps the case of no configured processes? So we could unify that call in the Supervisor, instead of having two separate checks?

Besides that, I think you can skip recurring_tasks.none? because if recurring_tasks is empty, recurring_tasks.all?(&:valid?) will return true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh... seems so obvious after you pointed it out 😅

I hope you don't mind, but I've kept the two distinct error messages. It feels like the verbose invalid task error message would be helpful.

end

def invalid_tasks
recurring_tasks.select(&:invalid?)
end

def max_number_of_threads
# At most "threads" in each worker + 1 thread for the worker + 1 thread for the heartbeat task
workers_options.map { |options| options[:threads] }.max + 2
Expand Down Expand Up @@ -100,7 +108,7 @@ def dispatchers_options
def recurring_tasks
@recurring_tasks ||= recurring_tasks_config.map do |id, options|
RecurringTask.from_configuration(id, **options)
end.select(&:valid?)
end
end

def processes_config
Expand Down
18 changes: 17 additions & 1 deletion lib/solid_queue/supervisor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,27 @@ def start(**options)
configuration = Configuration.new(**options)

if configuration.configured_processes.any?
new(configuration).tap(&:start)
if configuration.valid?
new(configuration).tap(&:start)
else
abort_due_to_invalid_tasks(configuration)
end
else
abort "No workers or processed configured. Exiting..."
end
end

private
def abort_due_to_invalid_tasks(configuration)
error_messages = configuration.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")

abort "Invalid processes configured:\n#{error_messages}"
end
end

def initialize(configuration)
Expand Down
5 changes: 5 additions & 0 deletions test/dummy/config/recurring_with_invalid.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
periodic_store_result:
class: StoreResultJorrrrrrb
queue: default
args: [42, { status: "custom_status" }]
schedule: every second
2 changes: 1 addition & 1 deletion test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def write(...)
Logger::LogDevice.prepend(BlockLogDeviceTimeoutExceptions)

class ActiveSupport::TestCase
include ProcessesTestHelper, JobsTestHelper
include ConfigurationTestHelper, ProcessesTestHelper, JobsTestHelper

teardown do
JobBuffer.clear
Expand Down
7 changes: 7 additions & 0 deletions test/test_helpers/configuration_test_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# frozen_string_literal: true

module ConfigurationTestHelper
def config_file_path(name)
Rails.root.join("config/#{name}.yml")
end
end
10 changes: 10 additions & 0 deletions test/test_helpers/processes_test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,16 @@ def run_supervisor_as_fork(**options)
end
end

def run_supervisor_as_fork_with_captured_io(**options)
pid = nil
out, err = capture_subprocess_io do
pid = run_supervisor_as_fork(**options)
wait_for_registered_processes(4)
end

[ pid, out, err ]
end

def wait_for_registered_processes(count, timeout: 1.second)
wait_while_with_timeout(timeout) { SolidQueue::Process.count != count }
end
Expand Down
24 changes: 20 additions & 4 deletions test/unit/configuration_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,26 @@ class ConfigurationTest < ActiveSupport::TestCase
assert_processes configuration, :dispatcher, 1, polling_interval: 0.1, recurring_tasks: nil
end

test "detects when there are invalid recurring tasks" do
configuration = SolidQueue::Configuration.new(recurring_schedule_file: config_file_path(:recurring_with_invalid))

assert_not configuration.valid?

assert_equal configuration.invalid_tasks.size, 1
end

test "is valid when there are no recurring tasks" do
configuration = SolidQueue::Configuration.new(recurring_schedule_file: config_file_path(:empty_recurring))

assert configuration.valid?
end

test "is valid when recurring tasks are skipped" do
configuration = SolidQueue::Configuration.new(skip_recurring: true)

assert configuration.valid?
end

private
def assert_processes(configuration, kind, count, **attributes)
processes = configuration.configured_processes.select { |p| p.kind == kind }
Expand Down Expand Up @@ -121,8 +141,4 @@ def assert_equal_value(expected_value, value)
assert_equal expected_value, value
end
end

def config_file_path(name)
Rails.root.join("config/#{name}.yml")
end
end
10 changes: 10 additions & 0 deletions test/unit/supervisor_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,16 @@ class SupervisorTest < ActiveSupport::TestCase
assert_not process_exists?(pid)
end

test "start with invalid configuration" do
pid, _out, err = run_supervisor_as_fork_with_captured_io(recurring_schedule_file: config_file_path(:recurring_with_invalid), skip_recurring: false)

sleep(0.5)
assert_no_registered_processes

assert_not process_exists?(pid)
assert_match %r{Invalid processes configured}, err
end

test "create and delete pidfile" do
assert_not File.exist?(@pidfile)

Expand Down