Skip to content
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.

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

Crons: croniter library does not correctly compute DST transition for some schedules #66868

Closed
evanpurkhiser opened this issue Mar 13, 2024 · 4 comments
Assignees

Comments

@evanpurkhiser
Copy link
Member

evanpurkhiser commented Mar 13, 2024

As reported here:

What's wrong

What is happening is schedules such as 0 5 * * * compute the wrong next_expected_checkin during DST transitions. This is a bug in the croniter library we're using.

Effect on customers

When DST starts we will compute next expected check-in an hour early, we then create a missed check-in for the monitor an hour early.

When DST ends we compute the next expected check-in an hour late. In this scenario we may not completely fail, since a customers check-in will simply come early (and because right now we don't treat early/late check-ins differently #44393 this is OK) it may re-compute the next check-in time to be the next correct occurrence.

After the first incorrect computation the next computation of the expected check-in time will be correct.

Upstream bug

This is a bug in croniter.

There are a number of reports to the library and it's forks:

Some work was done here to attempt to fix it, but it was never merged https://github.com/kiorky/croniter-fork/pull/34

It appears the original problem was introduced here taichino/croniter#92 Which incorrectly introduced this bug.

@evanpurkhiser
Copy link
Member Author

I've begun investigating this. My initial investigation looks like this actually has to do with the zoneinfo implementation we're using: kiorky/croniter#1 (comment)

@evanpurkhiser
Copy link
Member Author

I'm investigating moving us over to the cronsim library since this one is a much easier to follow implementation and uses zoneinfo so is much more likely to handle DST correctly.

@evanpurkhiser
Copy link
Member Author

Here's some test code I've written to see how many cron expressions are parsable

from datetime import datetime, UTC
from cronsim import CronSim

from croniter.croniter import croniter


def validate_cron_expressions(file_path):
    valid_count = 0
    invalid_expressions = []

    dt = datetime.now(UTC).replace(second=0, microsecond=0)

    print(f'It is {dt}')
    print('')

    try:
        with open(file_path, "r") as file:
            for line_num, expression in enumerate(file, start=1):
                cron_expr = expression.strip()
                try:

                    # Attempt to parse and simulate the cron expression
                    cron_sim = CronSim(cron_expr, dt)
                    cron_iter = croniter(cron_expr, dt)
                    valid_count += 1
                except Exception as e:
                    # Store invalid expressions with line number and error
                    invalid_expressions.append((line_num, cron_expr, str(e)))

        # Print results
        print(f"Total valid expressions: {valid_count}")
        if invalid_expressions:
            print("\nInvalid expressions:")
            for line_num, cron_expr, error in invalid_expressions:
                print(f"Line {line_num}: '{cron_expr}' - Error: {error}")
        else:
            print("No invalid expressions found.")

    except FileNotFoundError:
        print("The specified file was not found.")


# Replace 'cron_expressions.txt' with your actual file path
validate_cron_expressions("crons.txt")

For all our currently existing (~ 7K unique cron expressions) here's what cronsim can't parse

Invalid expressions:
Line 153: '00 15 * * TUE-SUN' - Error: Bad day-of-week
Line 166: '00 16 * * TUE-SUN' - Error: Bad day-of-week
Line 173: '00 17 * * TUE-SUN' - Error: Bad day-of-week
Line 484: '0 10 * * TUE-SUN' - Error: Bad day-of-week
Line 664: '0 12 * * TUE-SUN' - Error: Bad day-of-week
Line 764: '0 14 * * TUE-SUN' - Error: Bad day-of-week
Line 834: '0 15 * * tue-sun' - Error: Bad day-of-week
Line 835: '0 15 * * TUE-SUN' - Error: Bad day-of-week
Line 1426: '0 4 * * mon-sun' - Error: Bad day-of-week
Line 1549: '0/5 * * * 5-0' - Error: Bad day-of-week
Line 1999: '0 9 * * TUE-SUN' - Error: Bad day-of-week
Line 2192: '10 4 * * mon-sun' - Error: Bad day-of-week
Line 2201: '*/1 05-23 * * 6-0' - Error: Bad day-of-week
Line 2661: '1 4 * * mon-sun' - Error: Bad day-of-week
Line 2734: '15 10 * * TUE-SUN' - Error: Bad day-of-week
Line 2801: '15 15 * * TUE-SUN' - Error: Bad day-of-week
Line 2807: '15 16 * * TUE-SUN' - Error: Bad day-of-week
Line 2901: '15 4 * * mon-sun' - Error: Bad day-of-week
Line 2937: '15 6,12 * * MON-SUN' - Error: Bad day-of-week
Line 3052: '15 9 * * TUE-SUN' - Error: Bad day-of-week
Line 4219: '30 10 * * TUE-SUN' - Error: Bad day-of-week
Line 4251: '30 11 * * TUE-SUN' - Error: Bad day-of-week
Line 4290: '30 13 * * TUE-SUN' - Error: Bad day-of-week
Line 4304: '30 14 * * TUE-SUN' - Error: Bad day-of-week
Line 4322: '30 15 * * TUE-SUN' - Error: Bad day-of-week
Line 4340: '30 16 * * TUE-SUN' - Error: Bad day-of-week
Line 4497: '30 4 * * mon-sun' - Error: Bad day-of-week
Line 4651: '30 9 * * TUE-SUN' - Error: Bad day-of-week
Line 4973: '35 4 * * mon-sun' - Error: Bad day-of-week
Line 5014: '*/360 05-23 * * 6-0' - Error: Bad day-of-week
Line 5668: '45 13 * * TUE-SUN' - Error: Bad day-of-week
Line 5677: '45 14 * * TUE-SUN' - Error: Bad day-of-week
Line 5711: '45 15 * * TUE-SUN' - Error: Bad day-of-week
Line 5798: '45 21 * * mon-sun' - Error: Bad day-of-week
Line 5927: '45 9 * * TUE-SUN' - Error: Bad day-of-week
Line 5998: '47 21 * * mon-sun' - Error: Bad day-of-week

@evanpurkhiser
Copy link
Member Author

Okay.

Since we're running out of time until the DST end time (which is mostly inconsequential, since the computed time ends up just being an hour late) we were going to see if we could hack-in something in to skip misses during the standard time → daylight savings time transtion.

However, turns out we've already missed all those transitions in the southern hemisphere.

Here's some chat-gpt code that finds all the DST transitions starting 30 days back and looking ahead 60 days.

from datetime import datetime, timedelta
from zoneinfo import ZoneInfo, available_timezones


def find_upcoming_dst_transitions():
    # Set the time window (next 7 days)
    now = datetime.now() - timedelta(days=30)
    upcoming_transitions = []

    print(f'Start time is {now}')
    print('-----')

    # Check each timezone
    for timezone in available_timezones():
        tz = ZoneInfo(timezone)

        # Start with today at midnight in the specified timezone
        dt = now.replace(hour=0, minute=0, second=0, microsecond=0, tzinfo=tz)

        one_month = dt + timedelta(days=60)

        # Loop through each hour in the next week to detect transitions
        while dt < one_month:
            # Check if the DST offset changes between this hour and the next
            next_hour = dt + timedelta(hours=1)
            if dt.dst() != next_hour.dst():
                # Record the timezone and the exact transition time
                transition_type = "IN  to DST" if next_hour.dst() > dt.dst() else "OUT of DST"
                upcoming_transitions.append(
                    (timezone, next_hour, transition_type))
                break  # No need to check further within this timezone
            dt = next_hour

    return upcoming_transitions


# Example usage:
upcoming_dst_transitions = find_upcoming_dst_transitions()
upcoming_dst_transitions.sort(key=lambda k: k[2])

for tz, transition_time, transition_type in upcoming_dst_transitions:
    print(f"{transition_type}: {tz} at {transition_time}")

For the transition into DST, we already missed all of these

$ python upcoming_dst.py
Start time is 2024-10-02 16:18:17.245983
-----
IN  to DST: Antarctica/Macquarie at 2024-10-06 03:00:00+11:00
IN  to DST: Australia/Sydney at 2024-10-06 03:00:00+11:00
IN  to DST: Australia/Canberra at 2024-10-06 03:00:00+11:00
IN  to DST: Australia/Melbourne at 2024-10-06 03:00:00+11:00
IN  to DST: Australia/Currie at 2024-10-06 03:00:00+11:00
IN  to DST: Australia/ACT at 2024-10-06 03:00:00+11:00
IN  to DST: America/Asuncion at 2024-10-06 01:00:00-03:00
IN  to DST: Pacific/Norfolk at 2024-10-06 03:00:00+12:00
IN  to DST: Australia/Yancowinna at 2024-10-06 03:00:00+10:30
IN  to DST: Australia/Lord_Howe at 2024-10-06 03:00:00+11:00
IN  to DST: Australia/South at 2024-10-06 03:00:00+10:30
IN  to DST: Australia/NSW at 2024-10-06 03:00:00+11:00
IN  to DST: Australia/Broken_Hill at 2024-10-06 03:00:00+10:30
IN  to DST: Australia/Tasmania at 2024-10-06 03:00:00+11:00
IN  to DST: Australia/Hobart at 2024-10-06 03:00:00+11:00
IN  to DST: Australia/LHI at 2024-10-06 03:00:00+11:00
IN  to DST: Australia/Adelaide at 2024-10-06 03:00:00+10:30
IN  to DST: Australia/Victoria at 2024-10-06 03:00:00+11:00

And again, leaving DST isn't as important, since it just means we say the next checkin can happen an hour later than it should.

Going forward I am going to spend some time trying to migrate us to cronsim since that is a much more modern implementation of what we need.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant