From d23e35409a2cdb68645f1afa5a1d13bc58e8c7c2 Mon Sep 17 00:00:00 2001 From: Himanshu Kale Date: Thu, 15 Jan 2026 23:15:45 +0530 Subject: [PATCH 1/2] Fix: Detect and warn when schedules generate multiple CRONs Fixes #703 - Use Fugit::Nat.parse with multi: :fail to detect schedules that generate multiple CRONs - Raise validation error with clear message when multiple CRONs detected - Add tests for single CRON, multiple times (same/different minutes) - Prevents silent failures where only first CRON was used --- app/models/solid_queue/recurring_task.rb | 15 +++++++++++++-- .../models/solid_queue/recurring_task_test.rb | 19 +++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/app/models/solid_queue/recurring_task.rb b/app/models/solid_queue/recurring_task.rb index 2a776c8af..f2f6fbac3 100644 --- a/app/models/solid_queue/recurring_task.rb +++ b/app/models/solid_queue/recurring_task.rb @@ -103,7 +103,10 @@ def attributes_for_upsert private def supported_schedule - unless parsed_schedule.instance_of?(Fugit::Cron) + if parsed_schedule == :multiple_crons_detected + errors.add :schedule, :multiple_crons, + message: "generates multiple cron schedules. Please use separate recurring tasks for each schedule, or use explicit cron syntax (e.g., '40 0,15 * * *' for multiple times with the same minutes)" + elsif !parsed_schedule.instance_of?(Fugit::Cron) errors.add :schedule, :unsupported, message: "is not a supported recurring schedule" end end @@ -152,7 +155,15 @@ def arguments_with_kwargs def parsed_schedule - @parsed_schedule ||= Fugit.parse(schedule) + @parsed_schedule ||= begin + Fugit::Nat.parse(schedule, multi: :fail) || Fugit.parse(schedule) + rescue ArgumentError => e + if e.message.include?("multiple crons") + :multiple_crons_detected + else + raise + end + end end def job_class diff --git a/test/models/solid_queue/recurring_task_test.rb b/test/models/solid_queue/recurring_task_test.rb index 29e946c57..2de91d8c0 100644 --- a/test/models/solid_queue/recurring_task_test.rb +++ b/test/models/solid_queue/recurring_task_test.rb @@ -144,6 +144,25 @@ def perform assert_not SolidQueue::RecurringTask.new(key: "task-id", class_name: "SolidQueue::RecurringTaskTest::JobWithoutArguments").valid? end + test "schedules with multiple cron detection" do + task = recurring_task_with(class_name: "JobWithoutArguments", schedule: "every day at 00:40") + assert task.valid? + assert task.to_s.ends_with? "[ 40 0 * * * ]" + + task = recurring_task_with(class_name: "JobWithoutArguments", schedule: "every day at 00:40 and 15:40") + assert task.valid? + assert task.to_s.ends_with? "[ 40 0,15 * * * ]" + + task = recurring_task_with(class_name: "JobWithoutArguments", schedule: "every day at 00:40 and 15:20") + assert_not task.valid? + error_message = task.errors[:schedule].first + assert_includes error_message, "generates multiple cron schedules" + assert_includes error_message, "Please use separate recurring tasks" + + task = recurring_task_with(class_name: "JobWithoutArguments", schedule: "40 0 * * *") + assert task.valid? + end + test "valid and invalid job class and command" do # Command assert recurring_task_with(command: "puts '¡hola!'").valid? From cbd5fd314ef453aa35dd87056da994d9ace5102b Mon Sep 17 00:00:00 2001 From: Rosa Gutierrez Date: Wed, 18 Feb 2026 14:44:01 +0100 Subject: [PATCH 2/2] Tidy up schedule parsing and error handling --- app/models/solid_queue/recurring_task.rb | 32 +++++++++---------- .../models/solid_queue/recurring_task_test.rb | 10 +++--- 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/app/models/solid_queue/recurring_task.rb b/app/models/solid_queue/recurring_task.rb index f2f6fbac3..d248f9924 100644 --- a/app/models/solid_queue/recurring_task.rb +++ b/app/models/solid_queue/recurring_task.rb @@ -6,9 +6,9 @@ module SolidQueue class RecurringTask < Record serialize :arguments, coder: Arguments, default: [] - validate :supported_schedule + validate :ensure_schedule_supported validate :ensure_command_or_class_present - validate :existing_job_class + validate :ensure_existing_job_class scope :static, -> { where(static: true) } @@ -102,13 +102,19 @@ def attributes_for_upsert end private - def supported_schedule - if parsed_schedule == :multiple_crons_detected - errors.add :schedule, :multiple_crons, - message: "generates multiple cron schedules. Please use separate recurring tasks for each schedule, or use explicit cron syntax (e.g., '40 0,15 * * *' for multiple times with the same minutes)" - elsif !parsed_schedule.instance_of?(Fugit::Cron) + def ensure_schedule_supported + unless parsed_schedule.instance_of?(Fugit::Cron) errors.add :schedule, :unsupported, message: "is not a supported recurring schedule" end + rescue ArgumentError => error + message = if error.message.include?("multiple crons") + "generates multiple cron schedules. Please use separate recurring tasks for each schedule, " + + "or use explicit cron syntax (e.g., '40 0,15 * * *' for multiple times with the same minutes)" + else + error.message + end + + errors.add :schedule, :unsupported, message: message end def ensure_command_or_class_present @@ -117,7 +123,7 @@ def ensure_command_or_class_present end end - def existing_job_class + def ensure_existing_job_class if class_name.present? && job_class.nil? errors.add :class_name, :undefined, message: "doesn't correspond to an existing class" end @@ -155,15 +161,7 @@ def arguments_with_kwargs def parsed_schedule - @parsed_schedule ||= begin - Fugit::Nat.parse(schedule, multi: :fail) || Fugit.parse(schedule) - rescue ArgumentError => e - if e.message.include?("multiple crons") - :multiple_crons_detected - else - raise - end - end + @parsed_schedule ||= Fugit.parse(schedule, multi: :fail) end def job_class diff --git a/test/models/solid_queue/recurring_task_test.rb b/test/models/solid_queue/recurring_task_test.rb index 2de91d8c0..52312f12c 100644 --- a/test/models/solid_queue/recurring_task_test.rb +++ b/test/models/solid_queue/recurring_task_test.rb @@ -142,25 +142,25 @@ def perform # Empty schedule assert_not SolidQueue::RecurringTask.new(key: "task-id", class_name: "SolidQueue::RecurringTaskTest::JobWithoutArguments").valid? - end - test "schedules with multiple cron detection" do task = recurring_task_with(class_name: "JobWithoutArguments", schedule: "every day at 00:40") assert task.valid? assert task.to_s.ends_with? "[ 40 0 * * * ]" + task = recurring_task_with(class_name: "JobWithoutArguments", schedule: "40 0 * * *") + assert task.valid? + + # Single cron to represent both hours, as it's the same minute task = recurring_task_with(class_name: "JobWithoutArguments", schedule: "every day at 00:40 and 15:40") assert task.valid? assert task.to_s.ends_with? "[ 40 0,15 * * * ]" + # Would need two cron tabs to reprenset the different hours and minutes task = recurring_task_with(class_name: "JobWithoutArguments", schedule: "every day at 00:40 and 15:20") assert_not task.valid? error_message = task.errors[:schedule].first assert_includes error_message, "generates multiple cron schedules" assert_includes error_message, "Please use separate recurring tasks" - - task = recurring_task_with(class_name: "JobWithoutArguments", schedule: "40 0 * * *") - assert task.valid? end test "valid and invalid job class and command" do