From 3d2900ab694f032adb838add545fb2ba7f1349b0 Mon Sep 17 00:00:00 2001 From: Nicolas Meienberger Date: Sun, 8 Mar 2026 08:54:23 +0100 Subject: [PATCH] refactor: harden cron validation and fail early in case of wrong pattern --- app/server/core/__tests__/scheduler.test.ts | 145 ++++++++++++++++++ app/server/core/scheduler.ts | 45 ++++-- app/server/modules/backups/backups.service.ts | 15 +- bun.lock | 3 - package.json | 1 - 5 files changed, 186 insertions(+), 23 deletions(-) create mode 100644 app/server/core/__tests__/scheduler.test.ts diff --git a/app/server/core/__tests__/scheduler.test.ts b/app/server/core/__tests__/scheduler.test.ts new file mode 100644 index 00000000..9d63869e --- /dev/null +++ b/app/server/core/__tests__/scheduler.test.ts @@ -0,0 +1,145 @@ +import { afterEach, describe, expect, mock, spyOn, test, vi } from "bun:test"; +import { logger } from "~/server/utils/logger"; +import { Job, Scheduler } from "../scheduler"; + +const flushMicrotasks = async () => { + await Promise.resolve(); +}; + +const mockTimeZone = (timeZone: string) => { + const resolvedOptions = Intl.DateTimeFormat.prototype.resolvedOptions; + return spyOn(Intl.DateTimeFormat.prototype, "resolvedOptions").mockImplementation( + function (this: Intl.DateTimeFormat) { + return { ...resolvedOptions.call(this), timeZone }; + }, + ); +}; + +describe("Scheduler", () => { + afterEach(async () => { + await Scheduler.clear(); + mock.restore(); + vi.useRealTimers(); + }); + + test("keeps future cron ticks armed while a job is still running", async () => { + vi.useFakeTimers({ now: new Date("2026-03-08T00:00:00.000Z") }); + + let runCount = 0; + let releaseRun: (() => void) | undefined; + const warn = spyOn(logger, "warn"); + + class TestJob extends Job { + async run() { + runCount += 1; + await new Promise((resolve) => { + releaseRun = resolve; + }); + } + } + + Scheduler.build(TestJob).schedule("* * * * *"); + + expect(vi.getTimerCount()).toBe(1); + + vi.advanceTimersByTime(60_000); + await flushMicrotasks(); + + expect(runCount).toBe(1); + expect(vi.getTimerCount()).toBe(1); + + vi.advanceTimersByTime(60_000); + await flushMicrotasks(); + + expect(runCount).toBe(1); + expect(warn).toHaveBeenCalledWith("Skipping overlapping run for job TestJob"); + expect(vi.getTimerCount()).toBe(1); + + releaseRun?.(); + await flushMicrotasks(); + }); + + test("throws for invalid cron expressions instead of retrying with a fallback timer", () => { + vi.useFakeTimers({ now: new Date("2026-03-08T00:00:00.000Z") }); + + class InvalidCronJob extends Job { + async run() { + return; + } + } + + expect(() => Scheduler.build(InvalidCronJob).schedule("not a cron")).toThrow(); + expect(vi.getTimerCount()).toBe(0); + }); + + test("uses the local timezone instead of UTC when arming the next run", async () => { + mockTimeZone("America/New_York"); + vi.useFakeTimers({ now: new Date("2026-01-15T04:59:00.000Z") }); + + let runCount = 0; + + class MidnightJob extends Job { + async run() { + runCount += 1; + } + } + + Scheduler.build(MidnightJob).schedule("0 0 * * *"); + + vi.advanceTimersByTime(59_000); + await flushMicrotasks(); + expect(runCount).toBe(0); + + vi.advanceTimersByTime(1_000); + await flushMicrotasks(); + expect(runCount).toBe(1); + }); + + test("runs skipped-hour cron expressions once when daylight saving time jumps forward", async () => { + mockTimeZone("America/New_York"); + vi.useFakeTimers({ now: new Date("2026-03-08T06:59:00.000Z") }); + + let runCount = 0; + + class SpringForwardJob extends Job { + async run() { + runCount += 1; + } + } + + Scheduler.build(SpringForwardJob).schedule("0 2 * * *"); + + vi.advanceTimersByTime(60_000); + await flushMicrotasks(); + expect(runCount).toBe(1); + expect(vi.getTimerCount()).toBe(1); + + vi.advanceTimersByTime(60_000); + await flushMicrotasks(); + expect(runCount).toBe(1); + }); + + test("does not run the same repeated local time twice when daylight saving time falls back", async () => { + mockTimeZone("America/New_York"); + vi.useFakeTimers({ now: new Date("2026-11-01T05:20:00.000Z") }); + + let runCount = 0; + + class FallBackJob extends Job { + async run() { + runCount += 1; + } + } + + Scheduler.build(FallBackJob).schedule("30 1 * * *"); + + vi.advanceTimersByTime(10 * 60_000); + await flushMicrotasks(); + expect(runCount).toBe(1); + + vi.advanceTimersByTime(70 * 60_000); + await flushMicrotasks(); + expect(runCount).toBe(1); + expect(vi.getTimerCount()).toBe(1); + }); +}); diff --git a/app/server/core/scheduler.ts b/app/server/core/scheduler.ts index 5a45b526..ea6e9eff 100644 --- a/app/server/core/scheduler.ts +++ b/app/server/core/scheduler.ts @@ -11,29 +11,39 @@ class ScheduledTask { private timer: ReturnType | null = null; private active = true; private running = false; + private readonly timeZone = Intl.DateTimeFormat().resolvedOptions().timeZone || undefined; constructor( private readonly jobName: string, private readonly cronExpression: string, private readonly run: () => Promise, ) { + CronExpressionParser.parse(this.cronExpression); this.scheduleNext(); } private getDelay(fromDate: Date) { - try { - const interval = CronExpressionParser.parse(this.cronExpression, { - currentDate: fromDate, - tz: Intl.DateTimeFormat().resolvedOptions().timeZone, - }); - const nextRun = interval.next().toDate(); - return Math.max(0, nextRun.getTime() - Date.now()); - } catch (error) { - logger.error(`Failed to parse cron expression for ${this.jobName}:`, error); - return 60_000; + const interval = CronExpressionParser.parse(this.cronExpression, { + currentDate: fromDate, + tz: this.timeZone, + }); + const nextRun = interval.next().toDate(); + return Math.max(0, nextRun.getTime() - Date.now()); + } + + private clearTimer() { + if (this.timer) { + clearTimeout(this.timer); + this.timer = null; } } + private stopScheduling(error: unknown) { + this.active = false; + this.clearTimer(); + logger.error(`Stopping scheduled job ${this.jobName} after cron parsing failed:`, error); + } + private scheduleNext(fromDate = new Date()) { if (!this.active) return; @@ -45,10 +55,17 @@ class ScheduledTask { private async tick() { if (!this.active) return; + this.timer = null; + + try { + this.scheduleNext(new Date()); + } catch (error) { + this.stopScheduling(error); + return; + } if (this.running) { logger.warn(`Skipping overlapping run for job ${this.jobName}`); - this.scheduleNext(new Date()); return; } @@ -57,16 +74,12 @@ class ScheduledTask { await this.run(); } finally { this.running = false; - this.scheduleNext(new Date()); } } async stop() { this.active = false; - if (this.timer) { - clearTimeout(this.timer); - this.timer = null; - } + this.clearTimer(); } async destroy() { diff --git a/app/server/modules/backups/backups.service.ts b/app/server/modules/backups/backups.service.ts index 8c65588c..52c6a8bb 100644 --- a/app/server/modules/backups/backups.service.ts +++ b/app/server/modules/backups/backups.service.ts @@ -1,6 +1,15 @@ import { and, eq, inArray } from "drizzle-orm"; -import cron from "node-cron"; +import CronExpressionParser from "cron-parser"; import { NotFoundError, BadRequestError, ConflictError } from "http-errors-enhanced"; + +const isValidCron = (expression: string): boolean => { + try { + CronExpressionParser.parse(expression); + return true; + } catch { + return false; + } +}; import { db } from "../../db/db"; import { backupScheduleMirrorsTable, backupScheduleNotificationsTable, backupSchedulesTable } from "../../db/schema"; import type { CreateBackupScheduleBody, UpdateBackupScheduleBody, UpdateScheduleMirrorsBody } from "./backups.dto"; @@ -83,7 +92,7 @@ const getScheduleByIdOrShortId = async (idOrShortId: string | number) => { const createSchedule = async (data: CreateBackupScheduleBody) => { const organizationId = getOrganizationId(); - if (!cron.validate(data.cronExpression)) { + if (!isValidCron(data.cronExpression)) { throw new BadRequestError("Invalid cron expression"); } @@ -169,7 +178,7 @@ const updateSchedule = async (scheduleIdOrShortId: number | string, data: Update throw new NotFoundError("Backup schedule not found"); } - if (data.cronExpression && !cron.validate(data.cronExpression)) { + if (data.cronExpression && !isValidCron(data.cronExpression)) { throw new BadRequestError("Invalid cron expression"); } diff --git a/bun.lock b/bun.lock index bbd67fd0..4a5d9b81 100644 --- a/bun.lock +++ b/bun.lock @@ -55,7 +55,6 @@ "isbot": "^5.1.35", "lucide-react": "^0.577.0", "next-themes": "^0.4.6", - "node-cron": "^4.2.1", "qrcode.react": "^4.2.0", "react": "^19.2.4", "react-dom": "^19.2.4", @@ -1554,8 +1553,6 @@ "nitro": ["nitro@3.0.1-alpha.2", "", { "dependencies": { "consola": "^3.4.2", "crossws": "^0.4.3", "db0": "^0.3.4", "h3": "^2.0.1-rc.11", "jiti": "^2.6.1", "nf3": "^0.3.5", "ofetch": "^2.0.0-alpha.3", "ohash": "^2.0.11", "oxc-minify": "^0.110.0", "oxc-transform": "^0.110.0", "srvx": "^0.10.1", "undici": "^7.18.2", "unenv": "^2.0.0-rc.24", "unstorage": "^2.0.0-alpha.5" }, "peerDependencies": { "rolldown": ">=1.0.0-beta.0", "rollup": "^4", "vite": "^7 || ^8 || >=8.0.0-0", "xml2js": "^0.6.2" }, "optionalPeers": ["rolldown", "rollup", "vite", "xml2js"], "bin": { "nitro": "dist/cli/index.mjs" } }, "sha512-YviDY5J/trS821qQ1fpJtpXWIdPYiOizC/meHavlm1Hfuhx//H+Egd1+4C5SegJRgtWMnRPW9n//6Woaw81cTQ=="], - "node-cron": ["node-cron@4.2.1", "", {}, "sha512-lgimEHPE/QDgFlywTd8yTR61ptugX3Qer29efeyWw2rv259HtGBNn1vZVmp8lB9uo9wC0t/AT4iGqXxia+CJFg=="], - "node-domexception": ["node-domexception@1.0.0", "", {}, "sha512-/jKZoMpw0F8GRwl4/eLROPA3cfcXtLApP0QzLmUT/HuPCZWyB7IY9ZrMeKw2O/nFIqPQB3PVM9aYm0F312AXDQ=="], "node-fetch": ["node-fetch@3.3.2", "", { "dependencies": { "data-uri-to-buffer": "^4.0.0", "fetch-blob": "^3.1.4", "formdata-polyfill": "^4.0.10" } }, "sha512-dRB78srN/l6gqWulah9SrxeYnxeddIG30+GOqK/9OlLVyLg3HPnr6SqOWTWOXKRwC2eGYCkZ59NNuSgvSrpgOA=="], diff --git a/package.json b/package.json index f621f47d..b0600f0a 100644 --- a/package.json +++ b/package.json @@ -76,7 +76,6 @@ "isbot": "^5.1.35", "lucide-react": "^0.577.0", "next-themes": "^0.4.6", - "node-cron": "^4.2.1", "qrcode.react": "^4.2.0", "react": "^19.2.4", "react-dom": "^19.2.4",