mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
fix: disable a schedule whose cron has no run left instead of panicking (#11195)
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
e954d33613
commit
381d4470ef
@@ -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_tz::Tz>,
|
||||
) -> chrono::DateTime<chrono_tz::Tz> {
|
||||
) -> Result<chrono::DateTime<chrono_tz::Tz>> {
|
||||
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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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<Postgres>,
|
||||
) -> 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<String>) = 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
|
||||
// -----------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user