fix(scheduler): fail closed on malformed scheduled_time instead of 500 (#1410)
compute_next_run parsed scheduled_time as "HH:MM" with int(parts[0]), int(parts[1]) and no validation, so "9", "9am", "25:00", "9:" or ":30" raised IndexError/ValueError. The POST /tasks create route passes the user/LLM-supplied scheduled_time before its try block (and only validates the cron field), so a bad value surfaced as an unhandled 500 rather than the clean 400 used for other invalid fields — and the same crash could fire inside the scheduler loop when recomputing next_run for an already-stored bad row. Guard the parse and fail closed (warn + return None), matching the existing invalid-cron handling in the same function. Adds tests/test_scheduler_scheduled_time_validation.py — malformed values return None (fail before with IndexError/ValueError), valid HH:MM still computes.
This commit is contained in:
@@ -122,9 +122,18 @@ def compute_next_run(schedule: str, scheduled_time: str,
|
|||||||
if not scheduled_time:
|
if not scheduled_time:
|
||||||
return None
|
return None
|
||||||
|
|
||||||
# Parse HH:MM
|
# Parse HH:MM — fail closed on malformed input (no colon, non-numeric,
|
||||||
|
# out-of-range) the same way an invalid cron expression does above, so a
|
||||||
|
# bad value like "9" or "9am" returns None instead of raising IndexError/
|
||||||
|
# ValueError out of the create route (a 500) or the scheduler loop.
|
||||||
parts = scheduled_time.split(":")
|
parts = scheduled_time.split(":")
|
||||||
hour, minute = int(parts[0]), int(parts[1])
|
try:
|
||||||
|
hour, minute = int(parts[0]), int(parts[1])
|
||||||
|
if not (0 <= hour <= 23 and 0 <= minute <= 59):
|
||||||
|
raise ValueError("hour/minute out of range")
|
||||||
|
except (ValueError, IndexError):
|
||||||
|
logger.warning(f"Invalid scheduled_time '{scheduled_time}'")
|
||||||
|
return None
|
||||||
|
|
||||||
if schedule == "daily":
|
if schedule == "daily":
|
||||||
candidate = now.replace(hour=hour, minute=minute, second=0, microsecond=0)
|
candidate = now.replace(hour=hour, minute=minute, second=0, microsecond=0)
|
||||||
|
|||||||
26
tests/test_scheduler_scheduled_time_validation.py
Normal file
26
tests/test_scheduler_scheduled_time_validation.py
Normal file
@@ -0,0 +1,26 @@
|
|||||||
|
"""Regression: compute_next_run must fail closed on a malformed scheduled_time.
|
||||||
|
|
||||||
|
compute_next_run parsed scheduled_time as "HH:MM" with a bare
|
||||||
|
`int(parts[0]), int(parts[1])` and no validation, so a value like "9", "9am",
|
||||||
|
"25:00", "9:" or ":30" raised IndexError/ValueError. The POST /tasks create
|
||||||
|
route calls it with the user/LLM-supplied scheduled_time *before* its try block
|
||||||
|
(and only validates cron), so a bad value surfaced as an unhandled 500 instead
|
||||||
|
of a clean 400 — and the same crash could fire inside the scheduler loop when
|
||||||
|
recomputing next_run for an already-stored bad row.
|
||||||
|
|
||||||
|
Now it fails closed (returns None) like an invalid cron expression does.
|
||||||
|
"""
|
||||||
|
from datetime import datetime
|
||||||
|
|
||||||
|
from src.task_scheduler import compute_next_run
|
||||||
|
|
||||||
|
|
||||||
|
def test_malformed_scheduled_time_returns_none():
|
||||||
|
now = datetime(2026, 6, 2, 12, 0)
|
||||||
|
for bad in ("9", "9am", "09", "9:", ":30", "abc", "25:00", "09:99", ""):
|
||||||
|
assert compute_next_run("daily", bad, after=now) is None, bad
|
||||||
|
|
||||||
|
|
||||||
|
def test_valid_scheduled_time_still_computes():
|
||||||
|
now = datetime(2026, 6, 2, 8, 0)
|
||||||
|
assert compute_next_run("daily", "09:00", after=now) == datetime(2026, 6, 2, 9, 0)
|
||||||
Reference in New Issue
Block a user