feat(core): add timezone option for scheduled tasks - #5203
Conversation
Fixes vendurehq#5202. Adds an optional IANA timezone setting on SchedulerOptions (global) and ScheduledTaskConfig (per-task override). The effective timezone is resolved through a single shared helper used both by SchedulerService.createCronJob() and by StaleTaskService.getScheduleIntervalMs(), so the actual job cadence and the stale-lock interval computation cannot disagree. The identifier is validated at bootstrap with an error naming the task. Blank values are treated as unset, and the default remains undefined (process-local evaluation), so existing behaviour is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Vendure Core — View preview |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
biggamesmallworld
left a comment
There was a problem hiding this comment.
Thanks for this, and for the unusually thorough PR description. The core design is right: resolving the timezone through a single helper used by both SchedulerService.createCronJob() and StaleTaskService.getScheduleIntervalMs() is exactly what prevents the job cadence and the stale-lock interval from disagreeing. The Required<Omit<SchedulerOptions, 'timezone'>> & SchedulerOptions shape follows the existing entityOptions precedent rather than inventing a new one, and the tests assert exact instants instead of just checking for definedness. I ran the new specs locally: 19 passing.
Two things need to change before this can merge, plus a handful of smaller items.
Blocking
1. The docs file has an unrelated Prettier pass that breaks the Shiki highlight markers.
docs/docs/guides/developer-guide/scheduled-tasks/index.mdx picked up a wholesale reformat, and Prettier moved several // [!code highlight] comments onto their own lines:
- generateSitemapTask.configure({ // [!code highlight]
- params: { // [!code highlight]
+ generateSitemapTask.configure({
+ // [!code highlight]
+ params: {
+ // [!code highlight]The marker highlights the line it sits on and is then stripped, so generateSitemapTask.configure({ loses its highlight and the reader gets a highlighted blank line instead. Same damage at config.schedulerOptions.tasks.push(, SitemapPlugin.init({, and if (this.processContext.isWorker) {.
Please revert every hunk in that file that is not the new ## Timezones section, including the frontmatter quote style and the whitespace-only changes.
2. The timezone should be a field, not a suffix on scheduleDescription.
scheduleDescription: pattern ? `${cronstrue.toString(pattern)}${timezoneSuffix}` : 'unknown',scheduleDescription is a GraphQL field consumed by the dashboard table. Baking the timezone into it means any client that wants the value has to regex a parenthetical out of a cronstrue sentence. Please add timezone: String to the ScheduledTask GraphQL type and to TaskInfo, and leave scheduleDescription as the cron description alone. The UI can render the two together.
This also affects the existing assertion in packages/core/e2e/default-scheduler-plugin.e2e-spec.ts:86, which compares scheduleDescription to an exact string. It passes today only because no e2e config sets a timezone.
I realise your PR description lists this as a deliberate follow-up. I would rather have the field now than ship a string format that clients start parsing.
Should fix
Return the trimmed value. In getScheduleTimezone:
const timezone = taskTimezone?.trim() ? taskTimezone : schedulerOptions.timezone;
return timezone?.trim() ? timezone : undefined;The stated motivation for trimming is an environment variable carrying stray whitespace, but ' Europe/Stockholm ' passes both guards and is returned with the spaces intact. Intl then rejects it (Invalid time zone specified: Europe/Stockholm ) and bootstrap fails. It fails loudly rather than silently, so this is not a blocker, but it is the case that actually bites. Worth a spec case too: the current tests cover '' and ' ' only.
Validation is unreachable under the Noop strategy. assertValidTimezone is only called from createCronJob, and onApplicationBootstrap returns before the task loop when no scheduler strategy is configured. Low impact, since tasks do not run in that setup anyway, but the commit message says the identifier is validated at bootstrap and that is only conditionally true. Either move the validation ahead of the strategy check, or soften the claim.
The memoised interval is now DST-dependent. getScheduleIntervalMs caches into taskIntervalMap with no invalidation. Your own test shows a 25 hour interval computed near the Stockholm fall-back; compute it near spring-forward instead and a 23 hour interval is cached for the process lifetime, shortening the stale-lock threshold. It only affects tasks whose interval exceeds 23 hours, and it is arguably pre-existing for anyone running a non-UTC TZ, but this PR makes it reachable for UTC processes. Either cache the larger of the two gaps, or add a comment explaining why the drift is acceptable.
Lint. The four new files produce 6 import/order warnings. Please clear them.
No e2e coverage. The unit tests cover the resolution logic well, but nothing exercises the timezone through real DI wiring, where StaleTaskService just gained a ConfigService constructor parameter.
Trim the comments. The docblock on getScheduleTimezone reproduces the rationale from the commit message, which is where it belongs; one line saying the task timezone wins over the global one is enough. Three sentences narrate the change rather than describe the code, and should go: "preserving the behaviour of versions prior to the introduction of this option", "which preserves the behaviour of prior versions", and "When no timezone is configured, the behaviour is unchanged from prior versions." A reader arriving at these docs never saw the previous version.
Minor
if (timezone != null)beforeassertValidTimezone: the helper returnsstring | undefined, sonullcannot occur. A truthiness check is clearer.catch (e: any)bindseand never uses it. Usecatch {.assertValidTimezonevalidates againstIntl, but croner is what has to accept the value.Intlaccepts offset forms such as'+05:00'; if croner rejects those, you get the rawTypeErrorthis function exists to avoid. Either construct theCroninside the try, or document that only IANA names are supported.- Wrapping the
configure()signature across three lines leaked into the generated reference docs asPartial< Pick<.... Keep it on one line.
On naming: timezone is fine, no need to rename.
Description
Note
Authorship: This PR was implemented by Claude AI Fable 5 working under my direction, and reviewed by me before submission (the commit carries a
Co-Authored-Bytrailer).Fixes #5202.
Cron schedules for scheduled tasks are currently always evaluated in the process timezone, and the only way to influence that is the
TZenvironment variable — which conflicts with theTZ=UTCresolution of #3409 for correct naive-timestamp persistence. This PR adds an opt-in IANA timezone option:SchedulerOptions.timezone?: string— global default for all tasks.ScheduledTaskConfig.timezone?: string— per-task override, also settable viatask.configure({ timezone })for built-in tasks such ascleanSessionsTask.getScheduleTimezone()) resolves per-task → global →undefined, and is used by bothCronconstruction sites —SchedulerService.createCronJob()andStaleTaskService.getScheduleIntervalMs()— so the job cadence and the stale-lock/lock-hold interval computation cannot diverge.Invalid timezone "..." configured for scheduled task "...". Note that bootstrap already aborts on an invalid schedule today (croner throws insidenew Cronat construction), so this does not change the failure mode — it only makes the error name the offending task, consistent with theassert*-at-bootstrap pattern inConfigModule.onApplicationBootstrap().scheduleDescriptionreturned bygetTaskList()include the effective timezone (e.g.At 02:00 AM (Europe/Stockholm)), so the wall-clock time shown in the Admin UI is unambiguous. When no timezone is configured, both strings are unchanged.SchedulerOptions/ScheduledTask.No new dependency: croner
^10.0.1(already a dependency) supportsCronOptions.timezone; the change is a pass-through.RuntimeVendureConfig.schedulerOptionsuses theRequired<Omit<..., 'timezone'>> & ...pattern already established forentityOptions.entityIdStrategy, sincetimezonehas no default value.Naming: if you would prefer
cronTimezoneor similar, to keeptimezonefree for a future Channel/User-level concept (#3451, #4321), I am happy to rename.Verified locally: full
packages/coreunit suite passes (1459 tests, of which 19 are new), thedefault-scheduler-plugine2e suite passes against sqljs (which exercises the changedStaleTaskServiceconstructor through real DI), and the full monorepo build succeeds. The new specs were additionally run under five different process timezones (Europe/Stockholm,UTC,America/Chicago,Asia/Kolkata,Pacific/Chatham) to confirm process-TZ independence.Beyond the test suite: a functionally equivalent patch of the same two call sites (applied via pnpm patch against 3.7.1) has been running in a production store since late July — process in UTC, schedules evaluated in
Europe/Stockholm— without issues.Possible follow-up (deliberately out of scope to keep this PR small): exposing the effective timezone as a dedicated field on the
ScheduledTaskGraphQL type rather than as a suffix inscheduleDescription.Breaking changes
None at runtime — the default is
undefined, which preserves the current process-local evaluation exactly.One type-level note:
RuntimeVendureConfig['schedulerOptions'](and theConfigService#schedulerOptionsgetter) is no longer assignable toRequired<SchedulerOptions>, sincetimezonestays optional. Reads and writes of all existing members are unaffected.Screenshots
N/A (no UI changes).
Checklist
📌 Always:
👍 Most of the time:
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.