Skip to content

feat(core): add timezone option for scheduled tasks - #5203

Open
comega-johan wants to merge 1 commit into
vendurehq:minorfrom
comega-johan:feat/scheduler-timezone
Open

feat(core): add timezone option for scheduled tasks#5203
comega-johan wants to merge 1 commit into
vendurehq:minorfrom
comega-johan:feat/scheduler-timezone

Conversation

@comega-johan

@comega-johan comega-johan commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

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-By trailer).

Fixes #5202.

Cron schedules for scheduled tasks are currently always evaluated in the process timezone, and the only way to influence that is the TZ environment variable — which conflicts with the TZ=UTC resolution 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 via task.configure({ timezone }) for built-in tasks such as cleanSessionsTask.
  • One shared helper (getScheduleTimezone()) resolves per-task → global → undefined, and is used by both Cron construction sites — SchedulerService.createCronJob() and StaleTaskService.getScheduleIntervalMs() — so the job cadence and the stale-lock/lock-hold interval computation cannot diverge.
  • Blank values are treated as unset. An empty or whitespace-only string (e.g. from a defined-but-empty env var) falls through to the next level instead of failing validation, matching croner's own falsy-check semantics.
  • Bootstrap validation: an invalid identifier fails at startup with Invalid timezone "..." configured for scheduled task "...". Note that bootstrap already aborts on an invalid schedule today (croner throws inside new Cron at construction), so this does not change the failure mode — it only makes the error name the offending task, consistent with the assert*-at-bootstrap pattern in ConfigModule.onApplicationBootstrap().
  • Diagnosability: the "Registered scheduled task" log line and the scheduleDescription returned by getTaskList() 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.
  • Docs: new "Timezones" section in the scheduled-tasks guide, plus regenerated reference docs for SchedulerOptions / ScheduledTask.

No new dependency: croner ^10.0.1 (already a dependency) supports CronOptions.timezone; the change is a pass-through.

RuntimeVendureConfig.schedulerOptions uses the Required<Omit<..., 'timezone'>> & ... pattern already established for entityOptions.entityIdStrategy, since timezone has no default value.

Naming: if you would prefer cronTimezone or similar, to keep timezone free for a future Channel/User-level concept (#3451, #4321), I am happy to rename.

Verified locally: full packages/core unit suite passes (1459 tests, of which 19 are new), the default-scheduler-plugin e2e suite passes against sqljs (which exercises the changed StaleTaskService constructor 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 ScheduledTask GraphQL type rather than as a suffix in scheduleDescription.

Breaking changes

None at runtime — the default is undefined, which preserves the current process-local evaluation exactly.

One type-level note: RuntimeVendureConfig['schedulerOptions'] (and the ConfigService#schedulerOptions getter) is no longer assignable to Required<SchedulerOptions>, since timezone stays optional. Reads and writes of all existing members are unaffected.

Screenshots

N/A (no UI changes).

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains a single feature
  • I have checked my own PR

👍 Most of the time:

  • I have added or updated test cases
  • I have updated the README if needed

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

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-developer-hub

Copy link
Copy Markdown

Vendure CoreView preview

a7b7bb9

@vercel

vercel Bot commented Aug 28, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
vendure-storybook Ready Ready Preview Aug 28, 2026 10:35pm

Request Review

@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: dcbfb14a-c0f6-48b1-9748-0c058dcbd76b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@biggamesmallworld biggamesmallworld left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) before assertValidTimezone: the helper returns string | undefined, so null cannot occur. A truthiness check is clearer.
  • catch (e: any) binds e and never uses it. Use catch {.
  • assertValidTimezone validates against Intl, but croner is what has to accept the value. Intl accepts offset forms such as '+05:00'; if croner rejects those, you get the raw TypeError this function exists to avoid. Either construct the Cron inside the try, or document that only IANA names are supported.
  • Wrapping the configure() signature across three lines leaked into the generated reference docs as Partial< Pick<.... Keep it on one line.

On naming: timezone is fine, no need to rename.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants