diff --git a/nerve/cron/service.py b/nerve/cron/service.py index f8dfd02..a012a2d 100644 --- a/nerve/cron/service.py +++ b/nerve/cron/service.py @@ -59,6 +59,73 @@ def _parse_interval(interval: str) -> int: return total or 7200 # Default 2h +# Unix crontab day-of-week numbering is 0=Sun..6=Sat (7 also means Sun). +# APScheduler's numeric day_of_week is 0=Mon..6=Sun, and CronTrigger.from_crontab +# does NOT remap, so a numeric DOW like "1" (Unix Monday) gets read as APScheduler +# 1 = Tuesday, i.e. every numeric-DOW cron fires one weekday late. APScheduler does +# accept unambiguous three-letter day names, so we translate the numbers to names. +_UNIX_DOW_TO_NAME = { + 0: "sun", 1: "mon", 2: "tue", 3: "wed", + 4: "thu", 5: "fri", 6: "sat", 7: "sun", +} + + +def _remap_dow_value(value: str) -> str: + """Map a single Unix DOW number to an APScheduler day name. + + Non-numeric atoms (already a name like ``mon``, or ``*``) and numbers + outside 0-7 pass through unchanged so APScheduler can validate them. + """ + v = value.strip() + if v.isdigit() and int(v) in _UNIX_DOW_TO_NAME: + return _UNIX_DOW_TO_NAME[int(v)] + return v + + +def _remap_dow_atom(atom: str) -> str: + """Remap one comma-separated DOW atom, preserving range and step syntax. + + Handles ``*``, single values (``1``), ranges (``1-5``), and any of those + with a step suffix (``*/2``, ``1-5/2``). Only the numeric components are + translated; everything else is left intact. + """ + base, sep, step = atom.partition("/") + if base in ("*", ""): + remapped = base + elif "-" in base: + lo, _, hi = base.partition("-") + remapped = f"{_remap_dow_value(lo)}-{_remap_dow_value(hi)}" + else: + remapped = _remap_dow_value(base) + return f"{remapped}{sep}{step}" if sep else remapped + + +def _crontab_to_trigger(schedule: str) -> CronTrigger: + """Build a CronTrigger from a 5-field crontab string with Unix DOW semantics. + + Drop-in replacement for ``CronTrigger.from_crontab`` that fixes the + day-of-week off-by-one (see ``_UNIX_DOW_TO_NAME``). Only the DOW field is + treated differently; the other four fields and the no-explicit-timezone + behaviour are identical to ``from_crontab``. Raises ``ValueError`` for + anything that is not a 5-field expression, so interval strings like ``4h`` + keep falling through to the IntervalTrigger path. + """ + fields = schedule.split() + if len(fields) != 5: + raise ValueError(f"Not a 5-field crontab expression: {schedule!r}") + minute, hour, day, month, day_of_week = fields + remapped_dow = ",".join( + _remap_dow_atom(atom) for atom in day_of_week.split(",") + ) + return CronTrigger( + minute=minute, + hour=hour, + day=day, + month=month, + day_of_week=remapped_dow, + ) + + def _parse_timestamp(ts: str) -> datetime: """Parse a UTC timestamp string from the database into an aware datetime.""" if "T" not in ts: @@ -119,7 +186,7 @@ async def start(self) -> None: schedule_str = getattr(source_config, "schedule", "*/15 * * * *") try: - trigger = CronTrigger.from_crontab(schedule_str) + trigger = _crontab_to_trigger(schedule_str) except ValueError: seconds = _parse_interval(schedule_str) trigger = IntervalTrigger(seconds=seconds) @@ -178,7 +245,7 @@ async def _make_trigger(self, job: CronJob) -> CronTrigger | IntervalTrigger: the cadence survives restarts (persistent timer). """ try: - return CronTrigger.from_crontab(job.schedule) + return _crontab_to_trigger(job.schedule) except ValueError: pass @@ -228,7 +295,7 @@ async def _catchup_missed_jobs(self) -> None: def _is_overdue(job: CronJob, last_run: datetime, now: datetime) -> bool: """Check if a job should have fired between *last_run* and *now*.""" try: - trigger = CronTrigger.from_crontab(job.schedule) + trigger = _crontab_to_trigger(job.schedule) next_fire = trigger.get_next_fire_time(last_run, last_run) return next_fire is not None and next_fire < now except ValueError: diff --git a/tests/test_cron.py b/tests/test_cron.py index 0e3eaa3..655f2c5 100644 --- a/tests/test_cron.py +++ b/tests/test_cron.py @@ -10,7 +10,12 @@ import pytest_asyncio from nerve.cron.jobs import CronJob -from nerve.cron.service import CronService, _parse_interval, _parse_timestamp +from nerve.cron.service import ( + CronService, + _crontab_to_trigger, + _parse_interval, + _parse_timestamp, +) # --------------------------------------------------------------------------- @@ -118,6 +123,82 @@ def test_default_on_garbage(self): assert _parse_interval("???") == 7200 +# --------------------------------------------------------------------------- +# _crontab_to_trigger: Unix day-of-week semantics +# --------------------------------------------------------------------------- + +# A Saturday, so "next fire" lands on a distinct weekday for any DOW value. +_DOW_BASE = datetime(2026, 6, 20, 0, 0, tzinfo=timezone.utc) + + +def _fire_weekdays(schedule: str) -> set[str]: + """Collect the weekday abbreviations a crontab fires on within one week.""" + trigger = _crontab_to_trigger(schedule) + end = _DOW_BASE + timedelta(days=8) + days: set[str] = set() + prev = None + cur = _DOW_BASE + while True: + fire = trigger.get_next_fire_time(prev, cur) + if fire is None or fire > end: + break + days.add(fire.strftime("%a")) + prev = fire + # Jump to the start of the next day so per-minute schedules don't loop. + cur = fire.replace(hour=0, minute=0, second=0, microsecond=0) + timedelta(days=1) + return days + + +class TestCrontabToTrigger: + def test_numeric_monday_fires_monday(self): + """The bug: Unix DOW 1 (Monday) was firing Tuesday via from_crontab.""" + fire = _crontab_to_trigger("0 13 * * 1").get_next_fire_time(None, _DOW_BASE) + assert fire.strftime("%A") == "Monday" + assert (fire.hour, fire.minute) == (13, 0) + + def test_numeric_sunday_fires_sunday(self): + fire = _crontab_to_trigger("0 13 * * 0").get_next_fire_time(None, _DOW_BASE) + assert fire.strftime("%A") == "Sunday" + + def test_seven_also_means_sunday(self): + fire = _crontab_to_trigger("0 13 * * 7").get_next_fire_time(None, _DOW_BASE) + assert fire.strftime("%A") == "Sunday" + + def test_range_weekdays_only(self): + assert _fire_weekdays("* * * * 1-5") == {"Mon", "Tue", "Wed", "Thu", "Fri"} + + def test_list_monday_and_thursday(self): + assert _fire_weekdays("0 9 * * 1,4") == {"Mon", "Thu"} + + def test_star_dow_fires_every_day(self): + assert _fire_weekdays("0 9 * * *") == { + "Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun", + } + + def test_day_name_passthrough(self): + """Already-named DOW values are left intact (and stay correct).""" + fire = _crontab_to_trigger("0 13 * * mon").get_next_fire_time(None, _DOW_BASE) + assert fire.strftime("%A") == "Monday" + + @pytest.mark.parametrize( + "schedule", + ["0 5 * * *", "*/30 * * * *", "0 */4 * * *", "17 */4 * * *", "13 13 * * *"], + ) + def test_non_dow_fields_match_from_crontab(self, schedule): + """Schedules without a numeric DOW behave exactly like from_crontab.""" + from apscheduler.triggers.cron import CronTrigger + + ours = _crontab_to_trigger(schedule).get_next_fire_time(None, _DOW_BASE) + ref = CronTrigger.from_crontab(schedule).get_next_fire_time(None, _DOW_BASE) + assert ours == ref + + @pytest.mark.parametrize("schedule", ["4h", "30m", "1h30m", "???", ""]) + def test_non_crontab_raises_value_error(self, schedule): + """Interval strings must still raise so the IntervalTrigger path runs.""" + with pytest.raises(ValueError): + _crontab_to_trigger(schedule) + + # --------------------------------------------------------------------------- # _is_overdue # --------------------------------------------------------------------------- @@ -157,6 +238,19 @@ def test_interval_multiple_missed(self): last_run = _utc_now() - timedelta(hours=10) assert CronService._is_overdue(job, last_run, _utc_now()) is True + def test_weekly_overdue_after_exactly_one_week(self): + """A weekly Monday job is overdue one week later, not 6 or 8 days.""" + job = _make_job(schedule="0 13 * * 1") # Mondays 13:00 UTC + last_run = datetime(2026, 6, 15, 13, 0, tzinfo=timezone.utc) # a Monday + # Six days later (Sunday): the next Monday fire has not arrived yet. + assert CronService._is_overdue( + job, last_run, last_run + timedelta(days=6), + ) is False + # Just past the next Monday fire, so now overdue. + assert CronService._is_overdue( + job, last_run, last_run + timedelta(days=7, minutes=1), + ) is True + # --------------------------------------------------------------------------- # _make_trigger (interval alignment)