Improve get_smallest_cron_interval

[anuthebananu]

Summary & Motivation

Makes calculating the smallest time interval in a cron string deterministic for most common/simple cron strings. For more complex crons we fall back to the old sampling-based approach.

Also adds a guard against negative cron intervals to the sampling-based approach that can occur when the clocks roll back in a DST transition.

How I Tested These Changes

Lots of unit tests. Claude Code did a lot of heavy lifting here.

Changelog

Insert changelog entry or delete this section.

[anuthebananu]

• Improve get_smallest_cron_interval #32626 👈 (View in Graphite)
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

[anuthebananu]

moved these tests to a dedicated test file and consolidated with other tests

[gibsondan]

what bad thing happens if we remove the sampling-based approach entirely?

[anuthebananu]

With the current implementation, we would lose the ability to accurately determine the smallest cron interval for more complex cron strings such as 0 0 15 * 1 (15th of the month and mondays)

i can try to have claude improve the approach so it can handle more of these cases

[gibsondan]

what would the impact of that be in practice though? this is just to do some validation when the freshness policy is created right, do we need that to be there for all cron strings?

[gibsondan]

or could it happen later when the cronstring is actually evaluated maybe?

[anuthebananu]

Right now get_smallest_cron_interval is only called when instantiating a cron freshness policy as part of validation checks:

dagster/python_modules/dagster/dagster/_core/definitions/freshness.py

Lines 171 to 172 in 5a786ba

	smallest_cron_interval = get_smallest_cron_interval(deadline_cron)
	check.invariant(

specifically the validation is there to make sure lower_bound_delta fits within the smallest interval of the cron. lower_bound_delta is important because it denotes the earliest point prior to cron tick where the policy expects the asset to have materialized.

we allow the user to set the cron freshness policy's deadline_cron to any cron string recognized by Dagster, so we would need to be able to calculate the smallest cron interval for any of those cron strings

[anuthebananu]

we could definitely try a conservative "best guess" approach for the more complex cases - so for a cron string with time intervals of varying length, we make a conservative guess at the smallest time interval. it's not perfect but probably good enough for this use case.

[gibsondan]

i think aiming to get the sampling path out entirely would be a good idea. The main thing that troubles me about it is that it is called every time one of these objects is instantiated, including, for example, when we pull one out of the database when generating the asset graph in the daemon, for example. The fact that its non-deterministic is also a little troubling. For the first thing, if there was some way of distiniguishing between the codepaht when they are defined in code and the codepath when they are pulled out of the database that could also work.

[anuthebananu]

In CronFreshnessPolicy.__new__ we already distinguish between a fresh instantiation and reading a serialized policy: https://github.com/dagster-io/dagster/blob/1a339db7b417373e55438c29d50bef85fb7c741a/python_modules/dagster/dagster/_core/definitions/freshness.py#L156-L160

I can move the get_smallest_cron_interval call to only run on fresh instantiation, we'd just have to assume that any serialized policy has a valid lower_bound_interval.