Skip to content

Commit 5d0f138

Browse files
ryan-williamsclaude
andcommitted
is_fetch_due: raise clearly when croniter is missing
Previously fell through `except Exception: return True`, masking both missing-croniter and invalid-cron-expression cases — every `dvx run` silently treated the stage as due. Now: - Preset schedules (`daily`, `hourly`, `weekly`) work without croniter. - Cron expressions raise `RuntimeError` if croniter isn't installed, pointing at `dvx[cron]` extra or project-level install. - Invalid cron expressions raise `ValueError` with the bad expression. Spec: `specs/fetch-schedule-croniter-required.md`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 5761aab commit 5d0f138

2 files changed

Lines changed: 107 additions & 9 deletions

File tree

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
# `is_fetch_due` should fail loudly when croniter is missing
2+
3+
## Problem
4+
5+
`dvx.run.dvc_files.is_fetch_due` silently returns `True` if `croniter`
6+
isn't importable:
7+
8+
```python
9+
try:
10+
from croniter import croniter
11+
cron = croniter(schedule, last)
12+
next_fire = cron.get_next(datetime)
13+
...
14+
return now >= next_fire
15+
except Exception:
16+
# If croniter not installed or invalid expression, treat as due
17+
return True
18+
```
19+
20+
Since `croniter` is an *optional* extra (`dvx[cron]`) and isn't in the
21+
default install, every project that uses a cron-expression `schedule`
22+
(as opposed to a preset like `"daily"`) gets a silent schedule bypass:
23+
**every `dvx run` treats the stage as due**.
24+
25+
Downstream symptom in the `nj-crashes` project: a refresh stage with
26+
`schedule: "10 15 * * *"` fired on every dispatch — producing no-op
27+
"Refresh NJSP data" commits hours before the actual cron time.
28+
29+
## Proposal
30+
31+
### 1. Fail loudly if `croniter` is missing
32+
33+
`croniter` stays opt-in (`dvx[cron]` extra). Projects using cron
34+
expressions are expected to install it. Replace the bare
35+
`except Exception: return True` with targeted handling that fails loudly
36+
when croniter is missing but a cron expression is used:
37+
38+
```python
39+
interval = _SCHEDULE_INTERVALS.get(schedule)
40+
if interval is not None:
41+
return now >= last + interval # Preset, no croniter needed
42+
43+
# Cron expression — croniter is required
44+
try:
45+
from croniter import croniter
46+
except ImportError as e:
47+
raise RuntimeError(
48+
f"Cron-expression schedule {schedule!r} requires croniter. "
49+
"Install `dvx[cron]` or add croniter to your project deps."
50+
) from e
51+
52+
try:
53+
cron = croniter(schedule, last)
54+
except (ValueError, KeyError) as e:
55+
raise ValueError(f"Invalid cron expression in schedule: {schedule!r}") from e
56+
57+
next_fire = cron.get_next(datetime)
58+
if next_fire.tzinfo is None:
59+
next_fire = next_fire.replace(tzinfo=timezone.utc)
60+
return now >= next_fire
61+
```
62+
63+
Key changes:
64+
- Preset schedules (`"daily"`, `"hourly"`, `"weekly"`) never hit the
65+
croniter path — they work without croniter.
66+
- Cron expressions raise `RuntimeError` if croniter is missing (clear
67+
error message pointing at the fix).
68+
- Invalid cron expressions raise `ValueError` (currently they silently
69+
fall back to "always due").
70+
71+
### 2. Document
72+
73+
Add a `schedule` section to docs/README covering:
74+
- Preset names vs cron expressions
75+
- Cron expressions require `dvx[cron]` extra
76+
- Examples for common cases (daily at a specific time, multi-fire
77+
windows, etc.)
78+
79+
## Test plan
80+
81+
Add to `tests/test_dvc_files.py`:
82+
83+
1. Preset schedule + no croniter → works (mock out `croniter` import).
84+
2. Cron schedule + no croniter → `RuntimeError` with clear message.
85+
3. Invalid cron schedule → `ValueError` with the bad expression in msg.
86+
4. Valid cron schedule → behaves correctly before/after next_fire.
87+
5. Mixed case sanity: `"daily"`, `"0 15 * * *"`, `"*/10 15-16 * * *"`.
88+
89+
## Out of scope
90+
91+
- Adding a `freshness_check` hook (for "poll the source before deciding
92+
to run"). Worth a separate spec.
93+
- Expanding `schedule` to support executable/script values.

src/dvx/run/dvc_files.py

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -70,22 +70,27 @@ def is_fetch_due(schedule: str, last_run: str | None, now: datetime | None = Non
7070
if last.tzinfo is None:
7171
last = last.replace(tzinfo=timezone.utc)
7272

73-
# Simple interval schedules
73+
# Simple interval schedules — no croniter needed
7474
interval = _SCHEDULE_INTERVALS.get(schedule)
7575
if interval is not None:
7676
return now >= last + interval
7777

78-
# Cron expression: find next fire time after last_run
78+
# Cron expression — require croniter; raise on missing/invalid
7979
try:
8080
from croniter import croniter
81+
except ImportError as e:
82+
raise RuntimeError(
83+
f"Cron-expression schedule {schedule!r} requires croniter. "
84+
"Install `dvx[cron]` or add croniter to your project deps."
85+
) from e
86+
try:
8187
cron = croniter(schedule, last)
82-
next_fire = cron.get_next(datetime)
83-
if next_fire.tzinfo is None:
84-
next_fire = next_fire.replace(tzinfo=timezone.utc)
85-
return now >= next_fire
86-
except Exception:
87-
# If croniter not installed or invalid expression, treat as due
88-
return True
88+
except (ValueError, KeyError) as e:
89+
raise ValueError(f"Invalid cron expression in schedule: {schedule!r}") from e
90+
next_fire = cron.get_next(datetime)
91+
if next_fire.tzinfo is None:
92+
next_fire = next_fire.replace(tzinfo=timezone.utc)
93+
return now >= next_fire
8994

9095

9196
# Cache for git blob SHAs (keyed by (repo_path, ref))

0 commit comments

Comments
 (0)