mirror of
https://github.com/moltbot/moltbot.git
synced 2026-03-21 16:41:56 +00:00
fix(cron): use maintenance recompute after job execution to prevent 48h skip (#17852)
Root cause: after executing due jobs, onTimer called recomputeNextRuns() which advances any past-due nextRunAtMs to the next occurrence. If a job became due between findDueJobs and the post-execution locked block, its nextRunAtMs would be silently advanced without execution — causing daily schedules to jump 48h instead of 24h. Fix: replace recomputeNextRuns with recomputeNextRunsForMaintenance in the post-execution block. The maintenance variant only fills in missing nextRunAtMs values and never overwrites existing (including past-due) ones, ensuring no job runs are silently skipped. Closes #17852 Signed-off-by: pierreeurope <pierre.europe@pm.me>
This commit is contained in:
101
src/cron/service.issue-17852-daily-skip.test.ts
Normal file
101
src/cron/service.issue-17852-daily-skip.test.ts
Normal file
@@ -0,0 +1,101 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import type { CronServiceState } from "./service/state.js";
|
||||
import type { CronJob } from "./types.js";
|
||||
import { recomputeNextRuns, recomputeNextRunsForMaintenance } from "./service/jobs.js";
|
||||
|
||||
/**
|
||||
* Regression test for issue #17852: daily cron jobs skip a day (48h jump).
|
||||
*
|
||||
* Root cause: onTimer's results-processing block used the full
|
||||
* recomputeNextRuns which could silently advance a past-due nextRunAtMs
|
||||
* for a job that became due between findDueJobs and the post-execution
|
||||
* locked block — skipping that run and jumping 48h ahead.
|
||||
*
|
||||
* Fix: use recomputeNextRunsForMaintenance in the post-execution block,
|
||||
* which only fills in missing nextRunAtMs values and never overwrites
|
||||
* existing (including past-due) ones.
|
||||
*/
|
||||
describe("issue #17852 - daily cron jobs should not skip days", () => {
|
||||
const HOUR_MS = 3_600_000;
|
||||
const DAY_MS = 24 * HOUR_MS;
|
||||
|
||||
function createMockState(jobs: CronJob[], nowMs: number): CronServiceState {
|
||||
return {
|
||||
store: { version: 1, jobs },
|
||||
running: false,
|
||||
timer: null,
|
||||
storeLoadedAtMs: nowMs,
|
||||
deps: {
|
||||
storePath: "/mock/path",
|
||||
cronEnabled: true,
|
||||
nowMs: () => nowMs,
|
||||
log: {
|
||||
debug: () => {},
|
||||
info: () => {},
|
||||
warn: () => {},
|
||||
error: () => {},
|
||||
} as never,
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
it("recomputeNextRunsForMaintenance should NOT advance past-due nextRunAtMs", () => {
|
||||
// Simulate: job scheduled for 3:00 AM, timer processing happens at 3:00:01
|
||||
// The job was NOT executed in this tick (e.g., it became due between
|
||||
// findDueJobs and the post-execution block).
|
||||
const threeAM = Date.parse("2026-02-16T03:00:00.000Z");
|
||||
const now = threeAM + 1_000; // 3:00:01
|
||||
|
||||
const job: CronJob = {
|
||||
id: "daily-job",
|
||||
name: "daily 3am",
|
||||
enabled: true,
|
||||
schedule: { kind: "cron", expr: "0 3 * * *", tz: "UTC" },
|
||||
payload: { kind: "systemEvent", text: "daily task" },
|
||||
sessionTarget: "main",
|
||||
createdAtMs: threeAM - DAY_MS,
|
||||
updatedAtMs: threeAM - DAY_MS,
|
||||
state: {
|
||||
nextRunAtMs: threeAM, // Past-due by 1 second
|
||||
},
|
||||
};
|
||||
|
||||
const state = createMockState([job], now);
|
||||
recomputeNextRunsForMaintenance(state);
|
||||
|
||||
// Maintenance should NOT touch existing past-due nextRunAtMs.
|
||||
// The job should still be eligible for execution on the next timer tick.
|
||||
expect(job.state.nextRunAtMs).toBe(threeAM);
|
||||
});
|
||||
|
||||
it("full recomputeNextRuns WOULD silently advance past-due nextRunAtMs (the bug)", () => {
|
||||
// This test documents the buggy behavior that caused #17852.
|
||||
// The full recomputeNextRuns sees a past-due nextRunAtMs and advances it
|
||||
// to the next occurrence WITHOUT executing the job.
|
||||
const threeAM = Date.parse("2026-02-16T03:00:00.000Z");
|
||||
const now = threeAM + 1_000; // 3:00:01
|
||||
|
||||
const job: CronJob = {
|
||||
id: "daily-job",
|
||||
name: "daily 3am",
|
||||
enabled: true,
|
||||
schedule: { kind: "cron", expr: "0 3 * * *", tz: "UTC" },
|
||||
payload: { kind: "systemEvent", text: "daily task" },
|
||||
sessionTarget: "main",
|
||||
createdAtMs: threeAM - DAY_MS,
|
||||
updatedAtMs: threeAM - DAY_MS,
|
||||
state: {
|
||||
nextRunAtMs: threeAM, // Past-due by 1 second
|
||||
},
|
||||
};
|
||||
|
||||
const state = createMockState([job], now);
|
||||
recomputeNextRuns(state);
|
||||
|
||||
// The full recomputeNextRuns advances it to TOMORROW — skipping today's
|
||||
// execution entirely. This is the 48h jump bug: from the previous run
|
||||
// (yesterday 3 AM) to the newly computed next run (tomorrow 3 AM).
|
||||
const tomorrowThreeAM = threeAM + DAY_MS;
|
||||
expect(job.state.nextRunAtMs).toBe(tomorrowThreeAM);
|
||||
});
|
||||
});
|
||||
@@ -7,7 +7,6 @@ import { sweepCronRunSessions } from "../session-reaper.js";
|
||||
import {
|
||||
computeJobNextRunAtMs,
|
||||
nextWakeAtMs,
|
||||
recomputeNextRuns,
|
||||
recomputeNextRunsForMaintenance,
|
||||
resolveJobPayloadTextForMain,
|
||||
} from "./jobs.js";
|
||||
@@ -283,7 +282,12 @@ export async function onTimer(state: CronServiceState) {
|
||||
}
|
||||
}
|
||||
|
||||
recomputeNextRuns(state);
|
||||
// Use maintenance-only recompute to avoid advancing past-due
|
||||
// nextRunAtMs values that became due between findDueJobs and this
|
||||
// locked block. The full recomputeNextRuns would silently skip
|
||||
// those jobs (advancing nextRunAtMs without execution), causing
|
||||
// daily cron schedules to jump 48 h instead of 24 h (#17852).
|
||||
recomputeNextRunsForMaintenance(state);
|
||||
await persist(state);
|
||||
});
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user