From 381d4470ef699ea82283742132e56556b95d2bd2 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Thu, 17 Sep 2026 11:40:00 +0200 Subject: [PATCH] fix: disable a schedule whose cron has no run left instead of panicking (#11195) Co-authored-by: Claude Opus 5 --- backend/windmill-common/src/utils.rs | 43 ++++++++++++++++--- backend/windmill-queue/src/schedule.rs | 5 +-- backend/windmill-queue/tests/schedule_push.rs | 41 ++++++++++++++++++ 3 files changed, 79 insertions(+), 10 deletions(-) diff --git a/backend/windmill-common/src/utils.rs b/backend/windmill-common/src/utils.rs index 4b93bb89ff..fd0bd634ea 100644 --- a/backend/windmill-common/src/utils.rs +++ b/backend/windmill-common/src/utils.rs @@ -976,18 +976,33 @@ fn six_fields_hint(schedule_str: &str, version: Option<&str>, seconds_required: } impl ScheduleType { + /// `NotFound` means the expression has no run left (an expired year, an impossible + /// date), and schedule pushes disable the schedule on it. Every other error must stay + /// transient: croner fails across a DST jump longer than an hour (Antarctica/Troll) + /// and succeeds again once the jump has passed. pub fn find_next( &self, starting_from: &chrono::DateTime, - ) -> chrono::DateTime { + ) -> Result> { + let no_run_left = || { + Error::NotFound(format!( + "cron: the schedule has no run left after {}", + starting_from.format("%Y-%m-%d %H:%M:%S %Z") + )) + }; match self { ScheduleType::Croner(croner_schedule) => croner_schedule .find_next_occurrence(starting_from, false) - .expect("cron: a schedule should have a next event"), - ScheduleType::Cron(schedule) => schedule - .after(starting_from) - .next() - .expect("cron: a schedule should have a next event"), + .map_err(|e| match e { + croner::errors::CronError::TimeSearchLimitExceeded => no_run_left(), + e => Error::internal_err(format!( + "cron: could not compute the run after {}: {e}", + starting_from.format("%Y-%m-%d %H:%M:%S %Z") + )), + }), + ScheduleType::Cron(schedule) => { + schedule.after(starting_from).next().ok_or_else(no_run_left) + } } } @@ -1709,6 +1724,22 @@ mod tests { assert!(!err.contains("6 fields"), "{err}"); } + #[test] + fn find_next_reports_only_a_cron_with_no_run_left_as_not_found() { + use chrono::TimeZone; + let troll: chrono_tz::Tz = "Antarctica/Troll".parse().unwrap(); + // Troll's clocks jump from 01:00 to 03:00 on the last Sunday of March. + let before_jump = troll.with_ymd_and_hms(2027, 3, 28, 0, 30, 0).unwrap(); + + let expired = ScheduleType::from_str("0 0 9 1 1 * 2026", Some("v1"), true).unwrap(); + let err = expired.find_next(&before_jump).unwrap_err(); + assert!(matches!(err, Error::NotFound(_)), "{err}"); + + let across_jump = ScheduleType::from_str("0 30 1 * * *", Some("v2"), true).unwrap(); + let err = across_jump.find_next(&before_jump).unwrap_err(); + assert!(!matches!(err, Error::NotFound(_)), "{err}"); + } + /// A worker that restarts must land on the exact same name to reclaim its `worker_ping` /// row, while still never colliding with the other workers of its own process. The /// suffix must also stay a single `-` segment, which is what the interactive shell tag diff --git a/backend/windmill-queue/src/schedule.rs b/backend/windmill-queue/src/schedule.rs index fa495c9cd2..5d5cf005fc 100644 --- a/backend/windmill-queue/src/schedule.rs +++ b/backend/windmill-queue/src/schedule.rs @@ -166,13 +166,10 @@ pub async fn push_scheduled_job<'c>( } }; - let next = sched.find_next(&starting_from); - // println!("next event ({:?}): {}", tz, next); - // println!("next event(UTC): {}", next.with_timezone(&chrono::Utc)); + let next = sched.find_next(&starting_from)?; // Scheduled events must be stored in the database in UTC let next = next.with_timezone(&chrono::Utc); - // panic!("next: {}", next); let already_exists: bool = sqlx::query_scalar!( // Query plan: // - use of the `ix_v2_job_root_by_path` index; hence the `parent_job IS NULL` clause. diff --git a/backend/windmill-queue/tests/schedule_push.rs b/backend/windmill-queue/tests/schedule_push.rs index d58d9a5a46..333d211765 100644 --- a/backend/windmill-queue/tests/schedule_push.rs +++ b/backend/windmill-queue/tests/schedule_push.rs @@ -921,6 +921,47 @@ mod schedule_push { Ok(()) } + // ----------------------------------------------------------------------- + // try_schedule_next_job: a cron with no run left disables the schedule + // ----------------------------------------------------------------------- + + #[sqlx::test(migrations = "../migrations", fixtures("base", "schedule_push"))] + async fn test_cron_with_no_run_left_disables_schedule( + db: Pool, + ) -> anyhow::Result<()> { + sqlx::query( + "INSERT INTO schedule (workspace_id, path, edited_by, edited_at, schedule, timezone, enabled, script_path, is_flow, email, extra_perms, ws_error_handler_muted, no_flow_overlap, permissioned_as, cron_version) + VALUES ('test-workspace', 'f/system/test_schedule', 'test-user', now(), '0 0 9 1 1 * 2020', 'UTC', true, 'f/system/test_script', false, 'test@windmill.dev', '{}', false, false, 'u/test-user', 'v1')" + ) + .execute(&db) + .await?; + + let schedule = make_schedule(|s| { + s.schedule = "0 0 9 1 1 * 2020".to_string(); + s.cron_version = Some("v1".to_string()); + }); + let job = make_completed_job(&schedule); + + let tx = db.begin().await?; + let (tx, err) = + try_schedule_next_job(&db, tx, &job, &schedule, &schedule.script_path).await; + assert!(err.is_none(), "completion must go through, got: {err:?}"); + tx.commit().await?; + + assert_eq!(count_queued_jobs(&db).await, 0); + let (enabled, error): (bool, Option) = sqlx::query_as( + "SELECT enabled, error FROM schedule WHERE workspace_id = 'test-workspace' AND path = 'f/system/test_schedule'", + ) + .fetch_one(&db) + .await?; + assert!(!enabled, "schedule with no run left must be disabled"); + assert!( + error.as_deref().is_some_and(|e| e.contains("no run left")), + "error should say why, got: {error:?}" + ); + Ok(()) + } + // ----------------------------------------------------------------------- // try_schedule_next_job: disabled schedule leaves no side effects // -----------------------------------------------------------------------