fix: honor cron trigger timezone when scheduling runs - #7798
Open
anxkhn wants to merge 2 commits into
Open
Conversation
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a scheduling bug where cron triggers with a declared timezone were validated as timezone-aware but executed as UTC at runtime, causing triggers to fire at the wrong wall-clock time (including across DST transitions). It centralizes cron parsing so both deploy-time validation and runtime scheduling/catch-up calculations interpret schedules consistently.
Changes:
- Introduces a shared
schedule.ParseCronhelper that parses both structured cron schedules (honoringCron.timezoneviaCRON_TZ=) and legacycron_expression. - Updates the scheduler’s
ParseSchedulepath to use the shared parser so next-fire and catch-up times respect the declared timezone. - Updates trigger validation to use the same parsing logic, and adds tests covering timezone behavior and invalid cron expressions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| runs/service/trigger_service.go | Routes trigger cron validation through the shared cron parser for consistent validation. |
| runs/service/task_service_test.go | Adds table tests for cron validation across structured/legacy forms and invalid inputs. |
| runs/scheduler/core/scheduler_test.go | Adds scheduler tests verifying next-fire and catch-up behavior in UTC, non-UTC, and at a DST boundary. |
| runs/scheduler/core/schedule_time.go | Switches runtime schedule parsing to use the shared timezone-aware cron parser. |
| runs/schedule/cron.go | Adds the shared cron parsing helper that applies CRON_TZ for structured cron schedules with timezone. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+17
to
+33
| var expression string | ||
| switch value := schedule.GetExpression().(type) { | ||
| case *task.Schedule_CronExpression: | ||
| expression = value.CronExpression | ||
| case *task.Schedule_Cron: | ||
| expression = value.Cron.GetExpression() | ||
| if timezone := value.Cron.GetTimezone(); timezone != "" { | ||
| expression = fmt.Sprintf("CRON_TZ=%s %s", timezone, expression) | ||
| } | ||
| default: | ||
| return nil, nil | ||
| } | ||
|
|
||
| parsed, err := cron.ParseStandard(expression) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid cron expression %q: %w", expression, err) | ||
| } |
| if _, err := cron.ParseStandard(expr); err != nil { | ||
| if _, err := schedule.ParseCron(spec.GetSchedule()); err != nil { | ||
| return connect.NewError(connect.CodeInvalidArgument, | ||
| fmt.Errorf("trigger %q has invalid cron expression: %w", triggerName, err)) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why are the changes needed?
The structured
Cronschedule carries atimezone, and trigger validation already prependsCRON_TZ=before parsing it. The scheduler does not:ParseSchedulepasses onlyCron.expressiontocron.ParseStandard, and the cron runner defaults to UTC. Every accepted non-UTC trigger therefore executes in UTC.Concretely, a trigger deployed as
0 9 * * *with timezoneAmerica/Los_Angelespasses validation but fires at 09:00 UTC instead of 09:00 Pacific. Users get runs several hours early or late, the offset shifts again across daylight-saving transitions, and catch-up time calculations inherit the same error.CRON_TZis the per-schedule timezone override documented by robfig/cron, so the information is available, it is just dropped on the runtime path.What changes were proposed in this pull request?
ParseCronhelper that handles both schedule forms: the structuredCronmessage (applyingCRON_TZ=<timezone>when a timezone is set) and the legacycron_expressionstring.ParseScheduleso live scheduling and catch-up calculations respect the declared timezone.validateCronExpressionso deploy-time validation and runtime parsing can no longer disagree about what a schedule means.One behavior change worth calling out: because validation now goes through the same parser, an invalid legacy
cron_expressionis rejected at registration. Previously only the structuredCronform was validated, and a bad legacy expression was accepted and then failed later inside the scheduler.How was this patch tested?
New table tests in the scheduler core cover UTC, a non-UTC zone, and a DST boundary (7-8 March 2026 in
America/Los_Angeles), asserting both the next fire time and the catch-up times. New validation tests cover structured cron with a timezone, legacy cron, and invalid expressions in both forms.Check all the applicable boxes
Related PRs
None.
Docs link
None.