Skip to content

[BUG]Invalid cron date schedule creates infinite loop in flytescheduler #3312

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.

Sign up for GitHub

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
pingsutw merged 3 commits into from
Aug 16, 2025
Merged

[BUG]Invalid cron date schedule creates infinite loop in flytescheduler #3312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f20051
feature: add cron date check in flyte schedule
popojk Aug 14, 2025
d0d401f
fix: lint fix
popojk Aug 14, 2025
4faae88
fix: refactor code structure
popojk Aug 15, 2025
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
12 changes: 6 additions & 6 deletions flytekit/core/schedule.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,12 +142,12 @@ def _validate_expression(cron_expression: str):
def _validate_schedule(schedule: str):
if schedule.lower() not in CronSchedule._VALID_CRON_ALIASES:
try:
croniter.croniter(schedule)
except Exception:
raise ValueError(
"Schedule is invalid. It must be set to either a cron alias or valid cron expression."
f" Provided schedule: {schedule}"
)
# Validate the cron expression
cron = croniter.croniter(schedule)
# Try to get the next occurrence to validate the schedule
cron.get_next(datetime.datetime)
Copy link
Member

@machichima machichima Aug 14, 2025

Choose a reason for hiding this comment

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

Can we combine this check with above so that we do not need to have two try...except block

Copy link
Member

@machichima machichima Aug 14, 2025

Choose a reason for hiding this comment

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

btw is there any .validate() function that we can use instead of using get_next()?

Copy link
Contributor

@JiangJiaWei1103 JiangJiaWei1103 Aug 14, 2025

Choose a reason for hiding this comment

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

I suggest keeping them separate, and here's why:

  1. The first try block checks syntax: Whether schedule is a valid cron expression
  2. The second try block checks semantics: Whether the expression can generate valid dates

If they're combined together, the error handling would become generic and make it harder for users to pinpoint the actual problem.

@machichima, WDYT?

Copy link
Member

@machichima machichima Aug 14, 2025

Choose a reason for hiding this comment

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

Then how about adding Error: {str(e)} in the end of the error message?

While those two try...expect are just alike, I would still prefer merging them together. But I agree that we should print the error out here so that we know the actual problem

Copy link
Contributor

@JiangJiaWei1103 JiangJiaWei1103 Aug 14, 2025

Choose a reason for hiding this comment

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

Sounds reasonable, let's merge them together and throw out the error message so users can understand whether it's a syntax error or semantic one.

Copy link
Contributor Author

@popojk popojk Aug 15, 2025

Choose a reason for hiding this comment

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

@machichima @JiangJiaWei1103 Thank you both for the advice. I've merged two try..except blocks into one.

Copy link
Contributor Author

@popojk popojk Aug 15, 2025

Choose a reason for hiding this comment

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

btw is there any .validate() function that we can use instead of using get_next()?

@machichima I think the only way for croniter to validate cron date is calling get_next function. I'm not sure whether we can do the validation by others cron library, yet I prefer to keep using croniter as it is a simple way.

except Exception as e:
raise ValueError(f"Schedule is invalid. Provided schedule: {schedule} Error: {str(e)}")

@staticmethod
def _validate_offset(offset: str):
Expand Down
32 changes: 32 additions & 0 deletions tests/flytekit/unit/core/test_schedule.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,3 +153,35 @@ def quadruple(a: int) -> int:
assert lp.schedule == _schedule_models.Schedule(
"kickoff_input", rate=_schedule_models.Schedule.FixedRate(12, _schedule_models.Schedule.FixedRateUnit.HOUR)
)


@pytest.mark.parametrize(
"invalid_schedule",
[
"0 0 31 2 *", # February 31st (does not exist)
"0 0 30 2 *", # February 30th (does not exist)
"0 0 31 4 *", # April 31st (does not exist)
"0 0 31 6 *", # June 31st (does not exist)
],
)
def test_cron_invalid_date_combinations(invalid_schedule):
"""Test that CronSchedule rejects invalid date combinations like 31st of February."""
with pytest.raises(ValueError, match="Schedule is invalid."):
CronSchedule(schedule=invalid_schedule)


@pytest.mark.parametrize(
"valid_schedule",
[
"0 0 28 2 *", # February 28th (always valid)
"0 0 29 2 *", # February 29th (valid in leap years - handled by croniter)
"0 0 30 4 *", # April 30th (valid)
"0 0 31 1 *", # January 31st (valid)
"0 0 31 3 *", # March 31st (valid)
],
)
def test_cron_valid_date_combinations(valid_schedule):
"""Test that CronSchedule accepts valid date combinations."""
# These should not raise any exceptions
obj = CronSchedule(schedule=valid_schedule)
assert obj.cron_schedule.schedule == valid_schedule