diff --git a/CHANGELOG.md b/CHANGELOG.md index 936d70e9a4e..f91d9b4a4f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,7 @@ Docs: https://docs.openclaw.ai - Security/Agents: enforce workspace-root path bounds for `apply_patch` in non-sandbox mode to block traversal and symlink escape writes. Thanks @p80n-sec. - Cron/Slack: preserve agent identity (name and icon) when cron jobs deliver outbound messages. (#16242) Thanks @robbyczgw-cla. - Cron: deliver text-only output directly when `delivery.to` is set so cron recipients get full output instead of summaries. (#16360) Thanks @rubyrunsstuff. +- Cron: prevent `cron list`/`cron status` from silently skipping past-due recurring jobs by using maintenance recompute semantics. (#16156) Thanks @zerone0x. - Cron: repair missing/corrupt `nextRunAtMs` for the updated job without globally recomputing unrelated due jobs during `cron update`. (#15750) - Discord: prefer gateway guild id when logging inbound messages so cached-miss guilds do not appear as `guild=dm`. Thanks @thewilloftheshadow. - TUI: refactor searchable select list description layout and add regression coverage for ANSI-highlight width bounds. diff --git a/src/cron/service.issue-16156-list-skips-cron.test.ts b/src/cron/service.issue-16156-list-skips-cron.test.ts new file mode 100644 index 00000000000..a3f0069499a --- /dev/null +++ b/src/cron/service.issue-16156-list-skips-cron.test.ts @@ -0,0 +1,229 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { CronEvent } from "./service.js"; +import { CronService } from "./service.js"; + +const noopLogger = { + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), +}; + +async function makeStorePath() { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-cron-16156-")); + return { + storePath: path.join(dir, "cron", "jobs.json"), + cleanup: async () => { + await fs.rm(dir, { recursive: true, force: true }); + }, + }; +} + +function createFinishedBarrier() { + const resolvers = new Map void>(); + return { + waitForOk: (jobId: string) => + new Promise((resolve) => { + resolvers.set(jobId, resolve); + }), + onEvent: (evt: CronEvent) => { + if (evt.action !== "finished" || evt.status !== "ok") { + return; + } + const resolve = resolvers.get(evt.jobId); + if (!resolve) { + return; + } + resolvers.delete(evt.jobId); + resolve(evt); + }, + }; +} + +describe("#16156: cron.list() must not silently advance past-due recurring jobs", () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2025-12-13T00:00:00.000Z")); + noopLogger.debug.mockClear(); + noopLogger.info.mockClear(); + noopLogger.warn.mockClear(); + noopLogger.error.mockClear(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("does not skip a cron job when list() is called while the job is past-due", async () => { + const store = await makeStorePath(); + const enqueueSystemEvent = vi.fn(); + const requestHeartbeatNow = vi.fn(); + const finished = createFinishedBarrier(); + + const cron = new CronService({ + storePath: store.storePath, + cronEnabled: true, + log: noopLogger, + enqueueSystemEvent, + requestHeartbeatNow, + runIsolatedAgentJob: vi.fn(async () => ({ status: "ok" })), + onEvent: finished.onEvent, + }); + + await cron.start(); + + // Create a cron job that fires every minute. + const job = await cron.add({ + name: "every-minute", + enabled: true, + schedule: { kind: "cron", expr: "* * * * *" }, + sessionTarget: "main", + wakeMode: "next-heartbeat", + payload: { kind: "systemEvent", text: "cron-tick" }, + }); + + const firstDueAt = job.state.nextRunAtMs!; + expect(firstDueAt).toBe(Date.parse("2025-12-13T00:01:00.000Z")); + + // Advance time so the job is past-due but the timer hasn't fired yet. + vi.setSystemTime(new Date(firstDueAt + 5)); + + // Simulate the user running `cron list` while the job is past-due. + // Before the fix, this would call recomputeNextRuns() which silently + // advances nextRunAtMs to the next occurrence (00:02:00) without + // executing the job. + const listedBefore = await cron.list({ includeDisabled: true }); + const jobBeforeTimer = listedBefore.find((j) => j.id === job.id); + + // The job should still show the past-due nextRunAtMs, NOT the advanced one. + expect(jobBeforeTimer?.state.nextRunAtMs).toBe(firstDueAt); + + // Now let the timer fire. The job should be found as due and execute. + await vi.runOnlyPendingTimersAsync(); + + await finished.waitForOk(job.id); + + const jobs = await cron.list({ includeDisabled: true }); + const updated = jobs.find((j) => j.id === job.id); + + // Job must have actually executed. + expect(enqueueSystemEvent).toHaveBeenCalledWith("cron-tick", { + agentId: undefined, + }); + expect(updated?.state.lastStatus).toBe("ok"); + // nextRunAtMs must advance to a future minute boundary after execution. + expect(updated?.state.nextRunAtMs).toBeGreaterThan(firstDueAt); + + cron.stop(); + await store.cleanup(); + }); + + it("does not skip a cron job when status() is called while the job is past-due", async () => { + const store = await makeStorePath(); + const enqueueSystemEvent = vi.fn(); + const requestHeartbeatNow = vi.fn(); + const finished = createFinishedBarrier(); + + const cron = new CronService({ + storePath: store.storePath, + cronEnabled: true, + log: noopLogger, + enqueueSystemEvent, + requestHeartbeatNow, + runIsolatedAgentJob: vi.fn(async () => ({ status: "ok" })), + onEvent: finished.onEvent, + }); + + await cron.start(); + + const job = await cron.add({ + name: "five-min-cron", + enabled: true, + schedule: { kind: "cron", expr: "*/5 * * * *" }, + sessionTarget: "main", + wakeMode: "next-heartbeat", + payload: { kind: "systemEvent", text: "tick-5" }, + }); + + const firstDueAt = job.state.nextRunAtMs!; + + // Advance time past due. + vi.setSystemTime(new Date(firstDueAt + 10)); + + // Call status() while job is past-due. + await cron.status(); + + // Timer fires. + await vi.runOnlyPendingTimersAsync(); + + await finished.waitForOk(job.id); + + const jobs = await cron.list({ includeDisabled: true }); + const updated = jobs.find((j) => j.id === job.id); + + expect(enqueueSystemEvent).toHaveBeenCalledWith("tick-5", { + agentId: undefined, + }); + expect(updated?.state.lastStatus).toBe("ok"); + + cron.stop(); + await store.cleanup(); + }); + + it("still fills missing nextRunAtMs via list() for enabled jobs", async () => { + const store = await makeStorePath(); + const nowMs = Date.parse("2025-12-13T00:00:00.000Z"); + + // Write a store file with a cron job that has no nextRunAtMs. + await fs.mkdir(path.dirname(store.storePath), { recursive: true }); + await fs.writeFile( + store.storePath, + JSON.stringify( + { + version: 1, + jobs: [ + { + id: "missing-next", + name: "missing next", + enabled: true, + createdAtMs: nowMs, + updatedAtMs: nowMs, + schedule: { kind: "cron", expr: "* * * * *", tz: "UTC" }, + sessionTarget: "main", + wakeMode: "now", + payload: { kind: "systemEvent", text: "fill-me" }, + state: {}, + }, + ], + }, + null, + 2, + ), + "utf-8", + ); + + const cron = new CronService({ + storePath: store.storePath, + cronEnabled: true, + log: noopLogger, + enqueueSystemEvent: vi.fn(), + requestHeartbeatNow: vi.fn(), + runIsolatedAgentJob: vi.fn(async () => ({ status: "ok" })), + }); + + await cron.start(); + + // list() should fill in the missing nextRunAtMs via maintenance recompute. + const jobs = await cron.list({ includeDisabled: true }); + const job = jobs.find((j) => j.id === "missing-next"); + + expect(job?.state.nextRunAtMs).toBeTypeOf("number"); + expect(job?.state.nextRunAtMs).toBeGreaterThan(nowMs); + + cron.stop(); + await store.cleanup(); + }); +}); diff --git a/src/cron/service/ops.ts b/src/cron/service/ops.ts index 4a2d02f42d0..7a2aab6b724 100644 --- a/src/cron/service/ops.ts +++ b/src/cron/service/ops.ts @@ -8,6 +8,7 @@ import { isJobDue, nextWakeAtMs, recomputeNextRuns, + recomputeNextRunsForMaintenance, } from "./jobs.js"; import { locked } from "./locked.js"; import { ensureLoaded, persist, warnIfDisabled } from "./store.js"; @@ -53,7 +54,9 @@ export async function status(state: CronServiceState) { return await locked(state, async () => { await ensureLoaded(state, { skipRecompute: true }); if (state.store) { - const changed = recomputeNextRuns(state); + // Use the maintenance-only version so that read-only operations never + // advance a past-due nextRunAtMs without executing the job (#16156). + const changed = recomputeNextRunsForMaintenance(state); if (changed) { await persist(state); } @@ -71,7 +74,9 @@ export async function list(state: CronServiceState, opts?: { includeDisabled?: b return await locked(state, async () => { await ensureLoaded(state, { skipRecompute: true }); if (state.store) { - const changed = recomputeNextRuns(state); + // Use the maintenance-only version so that read-only operations never + // advance a past-due nextRunAtMs without executing the job (#16156). + const changed = recomputeNextRunsForMaintenance(state); if (changed) { await persist(state); }