Replace hardcoded maxCalendarYear with modified spec validation#9396
Replace hardcoded maxCalendarYear with modified spec validation#9396zivenrokhit wants to merge 17 commits intotemporalio:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes the maxCalendarYear for schedules configurable via dynamic config instead of being hardcoded to 2100. The change addresses customer needs for scheduling workflows beyond 2100 (e.g., for payment systems).
Changes:
- Added
SchedulerMaxCalendarYeardynamic config setting (default 2100) incommon/dynamicconfig/constants.go - Threaded
maxCalendarYearparameter throughSpecBuilder,CompiledSpec,compiledCalendar, and all parsing/validation functions - Wired dynamic config into the chasm scheduler FX module for injection at startup
- Updated all tests to explicitly pass
defaultMaxCalendarYearor2100as appropriate
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| common/dynamicconfig/constants.go | Added new global int setting SchedulerMaxCalendarYear with default value 2100 |
| service/worker/scheduler/calendar.go | Renamed constant to defaultMaxCalendarYear, added parameter to functions, threaded through compiledCalendar |
| service/worker/scheduler/spec.go | Added maxCalendarYear field to structs and threaded parameter through all spec processing functions |
| service/worker/scheduler/workflow.go | Updated SchedulerWorkflow to use defaultMaxCalendarYear (for tests/replays) |
| service/worker/scheduler/spec_test.go | Updated all test calls to pass defaultMaxCalendarYear |
| service/worker/scheduler/calendar_test.go | Updated all test calls to pass defaultMaxCalendarYear |
| service/frontend/workflow_handler_test.go | Updated test to pass explicit value 2100 to NewSpecBuilder |
| chasm/lib/scheduler/fx.go | Added newSpecBuilder function that reads dynamic config value and creates SpecBuilder |
| chasm/lib/scheduler/spec_processor_test.go | Updated test to pass explicit value 2100 to NewSpecBuilder |
| chasm/lib/scheduler/helper_test.go | Updated tests to pass explicit value 2100 to NewSpecBuilder |
| chasm/lib/scheduler/handler_test.go | Updated test to pass explicit value 2100 to NewSpecBuilder |
| chasm/lib/scheduler/generator_tasks_test.go | Updated test to pass explicit value 2100 to NewSpecBuilder |
dnr
left a comment
There was a problem hiding this comment.
I don't think this is a good solution (and the suggestion in the original bug report is misguided):
There's no problem with limiting actual execution of schedules to > 50 years in the future: the server will be updated and redeployed many times before that. The only real problem is that the execution limit is used in spec validation, when there's no need for that at all. That is, specs should be allowed to reference years farther in the future if they want. The matching times won't be calculated that far in the future (this is DoS protection, basically), but as soon as an updated server is deployed, they will be.
The fix should be to just stop imposing a maximum year validation on specs.
- Allow specs to reference years up to 9999 - Maintain DoS protection in iterator at year 2100 - Update tests to verify far-future years work - Remove obsolete year 2112 validation error test
|
Thanks for the feedback, I see what you are saying. I've scrapped the initial solution and relaxed the spec validation to allow for years beyond 2100, while keeping the match time DoS protection in place. I just want to highlight one possible edge case, if a user creates a schedule where the very next run isn't until after 2100, I believe the iterator will hit the DoS limit, fail to find a match, and immediately close the schedule as completed. Are we okay with this? |
|
@zivenrokhit could you update the title and description? |
Done |
|
Please add functional tests covering the RPCs impacted by relaxing spec validation beyond year 2100: 1. Create a weekly schedule, then call 2. Create a schedule with a spec that spans beyond 3. Create a schedule where all action times are beyond the calculation bound (e.g. 4. Create a schedule with a simple spec, then update it to a calendar spec referencing years beyond Place these on Unit tests to add in
|
Add tests covering the relaxed spec validation that allows years beyond 2100. Exports MaxCalendarYear and MaxListMatchingTimesCount constants for use in test assertions. Unit tests (spec_test.go): - TestSpecMaxSpecYearBoundary: year 9999 accepted, 10000 rejected - TestSpecYearStraddlesMaxCalendarYear: year range 2098-2150 computes through 2100 then stops - TestSpecFullYearRange: year range 2000-9999 accepted, computation stops at MaxCalendarYear Functional tests (schedule_test.go): - testListScheduleMatchingTimesWeeklyFarFuture: weekly schedule queried over far-future range, results capped at MaxListMatchingTimesCount - testDescribeScheduleFarFutureActionTimes: FutureActionTimes within MaxCalendarYear bound - testDescribeScheduleBeyondMaxCalendarYear: year 2150 schedule returns empty FutureActionTimes - testUpdateScheduleFarFutureSpec: update to far-future year range succeeds with FutureActionTimes within bound
done |
lina-temporal
left a comment
There was a problem hiding this comment.
some small comments on tests, otherwise LGTM. Let's get @dnr's stamp as well.
| // testDescribeScheduleFarFutureActionTimes verifies that DescribeSchedule returns | ||
| // FutureActionTimes within the MaxCalendarYear bound for a schedule that has no | ||
| // end date and will run indefinitely. | ||
| func testDescribeScheduleFarFutureActionTimes(t *testing.T, newContext contextFactory) { |
There was a problem hiding this comment.
I don't understand how this test is showing the "far future" (it doesn't appear to be really testing anything other than the immediate future), or really how it'd differentiate itself from any of the existing tests that validate FutureActionTimes.
There was a problem hiding this comment.
Added TestDescribeScheduleFutureActionTimesAtBoundary that should test what I was aiming at.
Remove redundant TestSpecYearStraddlesMaxCalendarYear (covered by TestSpecFullYearRange). Rework testDescribeScheduleFarFutureActionTimes into testDescribeScheduleFutureActionTimesAtBoundary that actually validates the MaxCalendarYear boundary. Replace hardcoded years with scheduler.MaxCalendarYear-relative expressions. Clean up unnecessary comments.
…values in spec tests
| // can safely increase this limit. | ||
| MaxCalendarYear = 2100 | ||
| // maxSpecYear is the largest year allowed in a schedule spec. This is set to a very | ||
| // large value to effectively remove the restriction while still preventing absurd values. |
There was a problem hiding this comment.
the wording "remove this restriction" only makes sense if you knew there was a restriction before.. word the comment so it make sense for readers reading the current state of the code
There was a problem hiding this comment.
good call, I've reworded it so that it doesn't reference the old restriction
| }) | ||
| s.NoError(err) | ||
|
|
||
| s.Eventually(func() bool { |
There was a problem hiding this comment.
use EventuallyWithT to make this more readable
| // MaxCalendarYear is the upper limit for calendar date computation. Specs can reference | ||
| // years beyond this limit, but actual calculation will stop here. Future server versions | ||
| // can safely increase this limit. | ||
| MaxCalendarYear = 2100 |
There was a problem hiding this comment.
did we consider something like, stopping the calendar search at ts + 100 years instead having a hardcoded limit? that might make things more predictable all over
There was a problem hiding this comment.
I see the point about a relative limit, but I think sticking with a fixed year is the better move here. It keeps the behavior deterministic and much simpler to test since the boundary is consistent for every call. A relative limit adds another moving part that could introduce edge cases or inconsistencies
…t and switched to eventuallyWithT for test
Decouple Spec Year Validation from Calendar Calculation Bound
Allow schedule specs to include years up to 9999, while keeping execution-time calendar search bounded at 2100 for safety.
What changed?
Why?
The previous approach used one limit for two different concerns:
This change separates those concerns:
Resolves #9383
How did you test it?
Potential risks