cc/cron: fix blacklist issues

[SoggySaussages]

The hour/day blacklists were causing difficulties, these fixes address those issues and add more thorough tests for
cron.

Currently, cron parse parses a * in hours the same as 0,1,2...,23, and similar for dow. This poses an issue since
0,1,2...,23 != *. Now if all hours/dows are set, the parse will leave the input unchanged rather than forcing "*"s to
"0,1,2...23"s.

If month/dom and dow are both configured, cron will process them using OR. If user didn't want that, they wouldn't set
both in the expression. but if using day of week blacklist, in the way we do it currently that would configure it, and
then cron would use OR and mess up the user's schedule if they explicitly avoided this behaviour in their expression.
For this reason, this commit now only uses the existing method if month/dom aren't configured, or if week is. Otherwise
it will loop recalculation of cron schedule until a new time is found which satisfies the dow blacklist.

Signed-off-by: SoggySaussages [email protected]

[jo3-l]

I would suggest removing all the tricky bit manipulation here and leave just the loop. (As in your original PR, the number of iterations can be bounded to a reasonable amount, and knowing Next is fairly efficient such an implementation should be fine.) The code, as is, seems unfortunately rather fragile now and is very tightly coupled to the cron internals--for instance, we need to know about 1<<63 (the special bit that cron uses to represent *), which is really an implementation detail.

In general, despite the tests, I do not think the correctness of the code is obvious--and even if it is, is it resilient to future changes to cron internals?--and would rather just always use the loop.

By the way, I know that I suggested reading off SpecSchedule originally in my comments on your original PR, but I intended that comment to apply only to parsing. I would have commented on this part of the implementation dissuading a rewrite, but given you had already written the code I figured just leaving it be would be fine. But now that it is proving much more tricky to get right than expected, I think the right move is to back out and use the simple, obviously correct loop. My sincere apologies for the confusion.

[SoggySaussages]

There was no confusion, you taught me a skill and I applied
it elsewhere that it was applicable. I don’t like only iterating,
if it’s limited to 100 iterations you can break it with
0,15,30,45 * * * * while blacklisting weekends. Double it
and it breaks when you throw in Friday. Double again and it
fails when blacklisting weekdays. I think the hybrid approach
is objectively the better option here, how do you feel about
me replacing magic numbers to improve readability and
adding back the hard limits?

[jo3-l]

if it’s limited to 100 iterations you can break it with
0,15,30,45 * * * * while blacklisting weekends. Double it
and it breaks when you throw in Friday. Double again and it
fails when blacklisting weekdays.

Good point. I did not think about the bound on the number of iterations precisely and underestimated the number. I think the only viable options are to go forward with the approach in this PR or to disable the blacklists for crons entirely. I prefer the latter--frankly crons with blacklists feel like significantly more trouble for little benefit--but perhaps the sail has shipped on that design decision. It is just a thought.

how do you feel about
me replacing magic numbers to improve readability and
adding back the hard limits?

I still remain uncomfortable with the approach in this PR. The magic numbers are not so much the problem; I am more concerned that they are needed at all. As I said earlier:

The code, as is, seems unfortunately rather fragile now and is very tightly coupled to the cron internals--for instance, we need to know about 1<<63 (the special bit that cron uses to represent *), which is really an implementation detail.

The tight coupling and reliance on implementation detail cannot be avoided by any refactoring.

That said, I do agree that if we want to retain support for blacklists, we should do it using the current approach here, not the loop.