You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "harjothkhara (via GitHub)" <gi...@apache.org> on 2026/07/19 04:40:12 UTC
[PR] Fix manual runs getting a zero-length data interval across DST [airflow]
harjothkhara opened a new pull request, #70089:
URL: https://github.com/apache/airflow/pull/70089
## Why
`CronMixin._get_prev` can return a tick that is not strictly earlier than the time it is asked about, so a manually triggered run gets a bad data interval on the two days a year the clocks change.
With `CronDataIntervalTimetable("0 2 * * *", timezone="America/New_York")`, a manual run at `2024-03-10 07:30Z`:
```
data interval: 2024-03-10 07:00Z -> 2024-03-10 07:00Z (zero length)
```
The scheduled path returns the correct `2024-03-09 07:00Z -> 2024-03-10 07:00Z` for the same period, so the two paths disagree about the same run.
croniter steps back on naive wall-clock time, but `make_aware` maps a tick that falls in a spring-forward gap onto the next real instant. When the previous tick is in the gap it lands on `current`, and `_get_prev` returns `current` itself.
The same remapping overshoots on the fall-back side. With `0 1 * * *` in `America/New_York` at `2024-11-03 05:30Z`, `_get_prev` returns `06:00Z` — half an hour in the future — and the inferred interval ends after `run_after`. `CronTriggerTimetable` with `run_immediately=True` picks that future tick as the run to fire now.
The zero-length interval has a second effect. `next_dagrun_info` treats a zero-length previous interval as a point-in-time run and advances one period (#66132, which is correct for the `CronTriggerTimetable` case it was written for). Fed this interval it skips a whole day:
```
after the buggy interval : 2024-03-11 06:00Z -> 2024-03-12 06:00Z
if the interval is right : 2024-03-10 07:00Z -> 2024-03-11 06:00Z
```
Also reproduces with `Europe/Zurich` `0 2 * * *` at 2023-03-26, and in `Australia/Sydney` and `Australia/Lord_Howe`.
## What
- `_get_prev` keeps stepping `cron.get_prev` until the converted result is strictly earlier than `current`. `get_prev` is strictly decreasing on naive time, so it terminates.
- The `_covers_every_hour` branch is left alone. It computes a wall-clock delta rather than going through `make_aware`, and the delta is strictly positive, so it cannot produce this.
- Docstring now states the strictly-earlier guarantee that callers already assume.
Over 23,040 sampled (cron, timezone, instant) combinations across eight timezones, 31 results change: 17 in `_get_prev`, 12 in `_align_to_prev`, 2 in `_align_to_next`. In every one the old value was already wrong — at or after the instant asked about, or a period skipped. Nothing outside a DST transition window changes.
I also checked `_get_next` for the mirror problem and did not find one — it never returned a value at or before its input across 7,680 sampled instants around transitions. It does skip the second occurrence of an ambiguous hour, but that is the existing `fold=1` policy in `make_aware` and is what `test_fold_scheduling` expects, so I left it out.
## Testing
Three regression tests, all failing before the change and passing after:
- `test_cron_infer_manual_data_interval_dst` — gap and fold for `America/New_York`, `Europe/Zurich` and `Australia/Lord_Howe` (a 30-minute offset). Asserts the interval is non-empty and does not end after `run_after`.
- `test_cron_next_dagrun_info_after_dst_manual_run` — the skipped period.
- `test_cron_trigger_run_immediately_does_not_pick_future_run` — `CronTriggerTimetable`, which shares the mixin.
`airflow-core/tests/unit/timetables/` is 246 passed.
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (please specify the tool below)
Generated-by: Claude Opus 4.8 following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix manual runs getting a zero-length data interval across DST [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #70089:
URL: https://github.com/apache/airflow/pull/70089#issuecomment-5014336780
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our [Contributors' Guide](https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [prek-hooks]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-prek-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/airflow-core/docs/howto/custom-operator.rst) Consider adding an example Dag that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#coding-style-and-best-practices).
- Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix manual runs getting a zero-length data interval across DST [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #70089:
URL: https://github.com/apache/airflow/pull/70089#discussion_r3744781764
##########
airflow-core/src/airflow/timetables/_cron.py:
##########
@@ -166,14 +166,21 @@ def _get_next(self, current: DateTime) -> DateTime:
return convert_to_utc(current.in_timezone(self._timezone) + delta)
def _get_prev(self, current: DateTime) -> DateTime:
- """Get the first schedule before specified time, with DST fixed."""
+ """Get the first schedule strictly before specified time, with DST fixed."""
naive = make_naive(current, self._timezone)
cron = croniter(self._expression, start_time=naive)
scheduled = cron.get_prev(datetime.datetime)
if TYPE_CHECKING:
assert isinstance(scheduled, datetime.datetime)
if not _covers_every_hour(cron):
- return convert_to_utc(make_aware(scheduled, self._timezone))
+ prev = convert_to_utc(make_aware(scheduled, self._timezone))
+ # croniter steps back on naive wall clock, but make_aware can map a tick inside
+ # a DST transition forward onto current or later. Keep stepping until strictly
+ # earlier; get_prev is strictly decreasing, so this terminates.
+ while prev >= current:
Review Comment:
Please add a parametrized case (mirroring `test_cron_infer_manual_data_interval_dst`) that drives `next_dagrun_info` through `_skip_to_latest` across a DST boundary, so this behavior doesn't silently regress if `_get_prev` changes again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix manual runs getting a zero-length data interval across DST [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #70089:
URL: https://github.com/apache/airflow/pull/70089#discussion_r3747215029
##########
airflow-core/tests/unit/timetables/test_interval_timetable.py:
##########
@@ -383,6 +383,173 @@ def test_cron_next_dagrun_info_alignment(last_data_interval: DataInterval, expec
assert info == expected_info
+@pytest.mark.parametrize(
+ ("timetable", "run_after", "expected_interval"),
+ [
+ pytest.param(
+ CronDataIntervalTimetable("0 2 * * *", pendulum.timezone("America/New_York")),
Review Comment:
Could we also add test cases to cover `MultipleCronTriggerTimetable` and `CronPartitionTimetable`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix manual runs getting a zero-length data interval across DST [airflow]
Posted by "harjothkhara (via GitHub)" <gi...@apache.org>.
harjothkhara commented on code in PR #70089:
URL: https://github.com/apache/airflow/pull/70089#discussion_r3745484912
##########
airflow-core/src/airflow/timetables/_cron.py:
##########
@@ -166,14 +166,21 @@ def _get_next(self, current: DateTime) -> DateTime:
return convert_to_utc(current.in_timezone(self._timezone) + delta)
def _get_prev(self, current: DateTime) -> DateTime:
- """Get the first schedule before specified time, with DST fixed."""
+ """Get the first schedule strictly before specified time, with DST fixed."""
naive = make_naive(current, self._timezone)
cron = croniter(self._expression, start_time=naive)
scheduled = cron.get_prev(datetime.datetime)
if TYPE_CHECKING:
assert isinstance(scheduled, datetime.datetime)
if not _covers_every_hour(cron):
- return convert_to_utc(make_aware(scheduled, self._timezone))
+ prev = convert_to_utc(make_aware(scheduled, self._timezone))
+ # croniter steps back on naive wall clock, but make_aware can map a tick inside
+ # a DST transition forward onto current or later. Keep stepping until strictly
+ # earlier; get_prev is strictly decreasing, so this terminates.
+ while prev >= current:
Review Comment:
Added a parametrized `test_cron_next_dagrun_info_no_catchup_dst` mirroring `test_cron_infer_manual_data_interval_dst`, driving `next_dagrun_info` through `_skip_to_latest` with `catchup=False`. All cases fail without the `_get_prev` change.
---
Drafted-by: Claude Code (Opus 5); reviewed by @harjothkhara before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix manual runs getting a zero-length data interval across DST [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #70089:
URL: https://github.com/apache/airflow/pull/70089#discussion_r3747013799
##########
airflow-core/newsfragments/70089.bugfix.rst:
##########
@@ -0,0 +1 @@
+Fix cron timetables inferring a data interval that is not strictly earlier than the run time on days a DST transition occurs. A manual run whose cron tick fell in a spring-forward gap got a zero-length interval, which also caused ``next_dagrun_info`` to skip the following scheduled period; on the fall-back side the inferred interval could end after ``run_after``, and ``CronTriggerTimetable`` with ``run_immediately`` could select a tick that had not happened yet.
Review Comment:
This is a bit long; suggest trimming to about 100-150 characters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix manual runs getting a zero-length data interval across DST [airflow]
Posted by "harjothkhara (via GitHub)" <gi...@apache.org>.
harjothkhara commented on code in PR #70089:
URL: https://github.com/apache/airflow/pull/70089#discussion_r3763737621
##########
airflow-core/tests/unit/timetables/test_trigger_timetable.py:
##########
@@ -1149,3 +1149,52 @@ def test_iter_partition_dagrun_infos_dst_america_new_york_spring_forward() -> No
# run_after == partition_date for both ticks.
for info in infos:
assert info.run_after == info.partition_date
+
+
+@time_machine.travel(pendulum.DateTime(2024, 11, 3, 5, 30, tzinfo=utc), tick=False)
Review Comment:
Merged the 3 tests into one parametrized `test_run_immediately_does_not_pick_future_run`, same DST fold case for each timetable.
---
Drafted-by: Claude Code (Opus 5); reviewed by @harjothkhara before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix manual runs getting a zero-length data interval across DST [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W merged PR #70089:
URL: https://github.com/apache/airflow/pull/70089
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix manual runs getting a zero-length data interval across DST [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #70089:
URL: https://github.com/apache/airflow/pull/70089#issuecomment-5263984635
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix manual runs getting a zero-length data interval across DST [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #70089:
URL: https://github.com/apache/airflow/pull/70089#discussion_r3755856277
##########
airflow-core/tests/unit/timetables/test_trigger_timetable.py:
##########
@@ -1149,3 +1149,52 @@ def test_iter_partition_dagrun_infos_dst_america_new_york_spring_forward() -> No
# run_after == partition_date for both ticks.
for info in infos:
assert info.run_after == info.partition_date
+
+
+@time_machine.travel(pendulum.DateTime(2024, 11, 3, 5, 30, tzinfo=utc), tick=False)
Review Comment:
we can simplify these 3 tests through parameterization
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org