Permit single time cron expressions #123

aegbert5 · 2025-10-08T22:26:47Z

Cron expressions such as * * * * * ? 1998 (every second of 1998) and 0 0 20 ? 10 WED#2 2025 (2025-10-08 Wed 20:00:00) should be permitted to run, however they require that triggerMaxErrorTime be set to a value greater than Long.MaxValue which is not possible. To allow for these cron expressions to run, we should permit any monitoring value >= 0 to be set

jdforde

Assuming that you tested this locally, but changes LGTM

aegbert5 · 2025-10-09T22:28:57Z

Manually tested cron expressions such as

0 31 16 8 10 ? 2126 - >100 years in the future, which fails the cron expression check (this is a known issue and does not relate to this change)
0 31 16 8 10 ? 2125 - <100 years in the future, which succeessfully is created if `monitoring-interval` is set to `Int.MaxValue`
0 31 16 8 10 ? 2125 - <100 years in the future, which fails to create if `monitoring-interval` is set to anything other than `Int.MaxValue`
* * * * * ? 1998 - A time period in the past, which fails to create, because quartz scheduler forbids it, no matter what the `monitoring-interval` is set to (this is a known issue and does not relate to this change)

tmccombs · 2025-10-10T20:50:17Z

Did you also test that you didn't break the behavior for recurring cron expressions?

aegbert5 · 2025-10-10T20:58:09Z

Did you also test that you didn't break the behavior for recurring cron expressions?

Yes, I have tested a few other basic cron expressions where the trigger is successfully created with the appropriate minimum window time, such as

7 7 7 * * ? - 7:07:07 a.m. every day - Minimum window time of 86401 seconds
7 7 * * * ? - The 7th minute of the hour - Minimum window time of 3601 seconds

There are also tons of cron expressions tested in the CronHelperTest.scala file that also validate this

tmccombs

First of all, it wasn't obvious to me why this solves the problem. It took me a while to figure out this just allows setting the monitor time to Int.maxValue to bypass the interval check. Which is definitely not obvious.

I think it would be better to have getMaxInterval return an Option, and if it is None, then we accept any value, but treat is Int.maxValue (or just disable monitoring).

tmccombs · 2025-10-10T20:54:27Z

admin/app/com/lucidchart/piezo/admin/utils/CronHelper.scala


 object CronHelper extends Logging {
  val IMPOSSIBLE_MAX_INTERVAL: Long = Long.MaxValue
+  val IMPOSSIBLE_MAX_INTERVAL_MINUS_ONE: Int = Int.MaxValue - 1


This name is confusing. What does it mean if the interval is set to this?
This isn't actually IMPOSSIBLE_MAX_INTERVAL - 1, since it is 1 less than the Int.MaxValue.

The name should instead indicate what this value acutally means. And maybe a comment to clarify.

I'm going to remove the IMPOSSIBLE_MAX_INTERVAL_MINUS_ONE variable in favor of using an Option

tmccombs · 2025-10-10T20:56:08Z

admin/app/com/lucidchart/piezo/admin/utils/CronHelper.scala

        val outermost = subexpressions(outermostIndex)
-        if (outermost.maxInterval == IMPOSSIBLE_MAX_INTERVAL) IMPOSSIBLE_MAX_INTERVAL
+        // When cron represents a past-time (or time too far in the future), the Long.MaxValue needs to be passed in
+        if (outermost.maxInterval == IMPOSSIBLE_MAX_INTERVAL) IMPOSSIBLE_MAX_INTERVAL_MINUS_ONE


I guess this MAX_INTERVAL isn't really impossible anymore?

I'm going to set these to None indicating that validation on the monitoring-interval wont take place

tmccombs · 2025-10-10T20:57:29Z

admin/app/com/lucidchart/piezo/admin/utils/CronHelper.scala

+
+  // Need to bound Long values to Int when validating, since the formatter expects Int type
+  private def longToIntBounded(l: Long): Int = {
+    if (l > Int.MaxValue) IMPOSSIBLE_MAX_INTERVAL_MINUS_ONE


Why not Int.MaxValue

I'm going to set these to None indicating that validation on the monitoring-interval wont take place

tmccombs · 2025-10-10T21:05:45Z

admin/test/com/lucidchart/piezo/admin/util/CronHelperTest.scala

      maxInterval("0/15 * * 1-12 * ?") mustEqual 19 * DAY + 15 * SECOND // every 15 seconds on days 1-12 of the month
      maxInterval("* * * * 1-11 ?") mustEqual 31 * DAY + SECOND // every second of every month except for december
-      maxInterval("* * * * * ? 1998") mustEqual IMPOSSIBLE // every second of 1998
+      maxInterval("* * * * * ? 1998") mustEqual IMPOSSIBLE_MINUS_ONE // every second of 1998


Ideally we probably want times in the past to be rejected, but times in the future are ok...

The behavior of piezo-admin will already reject cron expressions that only run in the past.

However, we should still test the validator for the monitoring-window for these crons even if they attempt to come through.

aegbert5 marked this pull request as draft October 9, 2025 15:19

aegbert5 marked this pull request as ready for review October 9, 2025 15:19

jdforde approved these changes Oct 9, 2025

View reviewed changes

aegbert5 force-pushed the permit-single-time-cron-expressions branch 2 times, most recently from 9884426 to 937b761 Compare October 9, 2025 22:24

aegbert5 force-pushed the permit-single-time-cron-expressions branch 5 times, most recently from 1879385 to af19d25 Compare October 10, 2025 20:14

tmccombs requested changes Oct 10, 2025

View reviewed changes

aegbert5 force-pushed the permit-single-time-cron-expressions branch from af19d25 to 51daf10 Compare October 10, 2025 21:59

Permit single time cron expressions

621d797

aegbert5 force-pushed the permit-single-time-cron-expressions branch from 51daf10 to 621d797 Compare October 10, 2025 22:00

aegbert5 requested review from jdforde and tmccombs October 10, 2025 22:41

jdforde approved these changes Oct 13, 2025

View reviewed changes

tmccombs approved these changes Oct 14, 2025

View reviewed changes

tmccombs merged commit e056cf9 into lucidsoftware:master Oct 14, 2025
1 check passed

Permit single time cron expressions #123

Permit single time cron expressions #123

Uh oh!

Conversation

aegbert5 commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdforde left a comment

Choose a reason for hiding this comment

Uh oh!

aegbert5 commented Oct 9, 2025

Uh oh!

tmccombs commented Oct 10, 2025

Uh oh!

aegbert5 commented Oct 10, 2025

Uh oh!

tmccombs left a comment

Choose a reason for hiding this comment

Uh oh!

tmccombs Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

aegbert5 Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

tmccombs Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

aegbert5 Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

tmccombs Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

aegbert5 Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

tmccombs Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

aegbert5 Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aegbert5 commented Oct 8, 2025 •

edited

Loading

aegbert5 Oct 10, 2025 •

edited

Loading