From 3224075edc21279454acc1cf5cdfbd7d12b4650e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=8B=90=E7=88=B7=26=26=E8=80=81=E6=8B=90=E7=98=A6?= Date: Thu, 30 Apr 2026 12:33:43 +0800 Subject: [PATCH] fix: reject invalid cron edits on disabled jobs (#74720) * fix(cron): reject invalid disabled schedule updates * docs: add cron validation changelog entry --------- Co-authored-by: Peter Steinberger --- CHANGELOG.md | 1 + src/cron/service.issue-regressions.test.ts | 55 ++++++++++++++++++++++ src/cron/service/ops.ts | 46 +++++++++++------- 3 files changed, 85 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50cf029dab4..3764146be38 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ Docs: https://docs.openclaw.ai ### Fixes - Config: accept documented `browser.tabCleanup` keys in strict root config validation, so configured tab cleanup no longer fails before runtime reads it. Fixes #74577. Thanks @lonexreb and @ezdlp. +- Cron: validate disabled job schedule edits before persisting updates, so invalid cron changes no longer partially mutate stored jobs. Fixes #74459. Thanks @yfge. - Channels/status: keep Telegram, Slack, and Google Chat read-only allowlist/default-target accessors on config-only paths, so status and channel summaries do not resolve SecretRef-backed runtime credentials. Thanks @eusine. - Active Memory: clarify the deprecated `modelFallbackPolicy` warning and config help so `modelFallback` is described as a chain-resolution last resort, not runtime failover. (#74602) Thanks @jeffrey701. - Channels/Discord: keep read-only allowlist/default-target accessors from resolving SecretRef-backed bot tokens, so status and channel summaries no longer fail when tokens are only available in gateway runtime. (#74737) Thanks @eusine. diff --git a/src/cron/service.issue-regressions.test.ts b/src/cron/service.issue-regressions.test.ts index 83aad3c0ae0..49595cb593b 100644 --- a/src/cron/service.issue-regressions.test.ts +++ b/src/cron/service.issue-regressions.test.ts @@ -260,6 +260,61 @@ describe("Cron issue regressions", () => { cron.stop(); }); + it("rejects invalid cron schedule updates without mutating disabled jobs", async () => { + const store = cronIssueRegressionFixtures.makeStorePath(); + const cron = await startCronForStore({ storePath: store.storePath, cronEnabled: false }); + + const disabledJob = await cron.add({ + name: "disabled-cron", + enabled: false, + schedule: { kind: "cron", expr: "0 * * * *", tz: "UTC" }, + sessionTarget: "main", + wakeMode: "next-heartbeat", + payload: { kind: "systemEvent", text: "tick" }, + }); + + await expect( + cron.update(disabledJob.id, { + schedule: { kind: "cron", expr: "* * * 13 *", tz: "UTC" }, + }), + ).rejects.toThrow("CronPattern"); + + let persisted = await loadCronStore(store.storePath); + let storedJob = persisted.jobs.find((job) => job.id === disabledJob.id); + expect(storedJob?.enabled).toBe(false); + expect(storedJob?.schedule).toEqual( + expect.objectContaining({ kind: "cron", expr: "0 * * * *", tz: "UTC" }), + ); + + await writeCronStoreSnapshot(store.storePath, [ + { + id: "invalid-disabled-job", + name: "invalid disabled job", + createdAtMs: Date.parse("2026-02-06T10:00:00.000Z"), + updatedAtMs: Date.parse("2026-02-06T10:00:00.000Z"), + enabled: false, + schedule: { kind: "cron", expr: "* * * 13 *", tz: "UTC" }, + sessionTarget: "main", + wakeMode: "next-heartbeat", + payload: { kind: "systemEvent", text: "tick" }, + state: {}, + }, + ]); + + const invalidCron = await startCronForStore({ storePath: store.storePath, cronEnabled: false }); + await expect(invalidCron.update("invalid-disabled-job", { enabled: true })).rejects.toThrow( + "CronPattern", + ); + + persisted = await loadCronStore(store.storePath); + storedJob = persisted.jobs.find((job) => job.id === "invalid-disabled-job"); + expect(storedJob?.enabled).toBe(false); + expect(storedJob?.state.nextRunAtMs).toBeUndefined(); + + invalidCron.stop(); + cron.stop(); + }); + it("keeps telegram delivery target writeback after manual cron.run", async () => { const store = cronIssueRegressionFixtures.makeStorePath(); const originalTarget = "https://t.me/obviyus"; diff --git a/src/cron/service/ops.ts b/src/cron/service/ops.ts index 9f3fa08d871..f8c352a44de 100644 --- a/src/cron/service/ops.ts +++ b/src/cron/service/ops.ts @@ -353,19 +353,20 @@ export async function update(state: CronServiceState, id: string, patch: CronJob await ensureLoaded(state, { skipRecompute: true }); const job = findJobOrThrow(state, id); const now = state.deps.nowMs(); - applyJobPatch(job, patch, { defaultAgentId: state.deps.defaultAgentId }); - if (job.schedule.kind === "every") { - const anchor = job.schedule.anchorMs; + const nextJob = structuredClone(job); + applyJobPatch(nextJob, patch, { defaultAgentId: state.deps.defaultAgentId }); + if (nextJob.schedule.kind === "every") { + const anchor = nextJob.schedule.anchorMs; if (typeof anchor !== "number" || !Number.isFinite(anchor)) { const patchSchedule = patch.schedule; const fallbackAnchorMs = patchSchedule?.kind === "every" ? now - : typeof job.createdAtMs === "number" && Number.isFinite(job.createdAtMs) - ? job.createdAtMs + : typeof nextJob.createdAtMs === "number" && Number.isFinite(nextJob.createdAtMs) + ? nextJob.createdAtMs : now; - job.schedule = { - ...job.schedule, + nextJob.schedule = { + ...nextJob.schedule, anchorMs: Math.max(0, Math.floor(fallbackAnchorMs)), }; } @@ -373,16 +374,27 @@ export async function update(state: CronServiceState, id: string, patch: CronJob const scheduleChanged = patch.schedule !== undefined; const enabledChanged = patch.enabled !== undefined; - job.updatedAtMs = now; + if (scheduleChanged && nextJob.schedule.kind === "cron" && !isJobEnabled(nextJob)) { + computeJobNextRunAtMs({ ...nextJob, enabled: true }, now); + } + + nextJob.updatedAtMs = now; if (scheduleChanged || enabledChanged) { - if (isJobEnabled(job)) { - job.state.nextRunAtMs = computeJobNextRunAtMs(job, now); + if (isJobEnabled(nextJob)) { + nextJob.state.nextRunAtMs = computeJobNextRunAtMs(nextJob, now); } else { - job.state.nextRunAtMs = undefined; - job.state.runningAtMs = undefined; + nextJob.state.nextRunAtMs = undefined; + nextJob.state.runningAtMs = undefined; + } + } else if (isJobEnabled(nextJob) && !hasScheduledNextRunAtMs(nextJob.state.nextRunAtMs)) { + nextJob.state.nextRunAtMs = computeJobNextRunAtMs(nextJob, now); + } + + if (state.store) { + const index = state.store.jobs.findIndex((entry) => entry.id === id); + if (index >= 0) { + state.store.jobs[index] = nextJob; } - } else if (isJobEnabled(job) && !hasScheduledNextRunAtMs(job.state.nextRunAtMs)) { - job.state.nextRunAtMs = computeJobNextRunAtMs(job, now); } await persist(state); @@ -390,10 +402,10 @@ export async function update(state: CronServiceState, id: string, patch: CronJob emit(state, { jobId: id, action: "updated", - job, - nextRunAtMs: job.state.nextRunAtMs, + job: nextJob, + nextRunAtMs: nextJob.state.nextRunAtMs, }); - return job; + return nextJob; }); }