Skip to content

Commit 40edb7f

Browse files
author
Peter Steinberger
committed
fix(doctor): persist legacy cron migration receipts
1 parent 9100d9b commit 40edb7f

6 files changed

Lines changed: 663 additions & 177 deletions

File tree

‎src/commands/doctor/cron/index.test.ts‎

Lines changed: 237 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
// Doctor cron index tests cover cron doctor checks and repair entrypoints.
2+
import fsSync from "node:fs";
23
import fs from "node:fs/promises";
34
import os from "node:os";
45
import path from "node:path";
@@ -14,14 +15,11 @@ import {
1415
} from "../../../cron/store.js";
1516
import { runOpenClawStateWriteTransaction } from "../../../state/openclaw-state-db.js";
1617
import { withRestoredMocks } from "../../../test-utils/vitest-spies.js";
17-
import { migrateLegacyDreamingPayloadShape } from "./dreaming-payload-migration.js";
1818
import {
1919
collectLegacyWhatsAppCrontabHealthWarning,
2020
maybeRepairLegacyCronStore,
2121
noteLegacyWhatsAppCrontabHealthCheck,
2222
} from "./index.js";
23-
import { migrateLegacyNotifyFallback } from "./legacy-notify.js";
24-
import { normalizeStoredCronJobs } from "./store-migration.js";
2523

2624
type TerminalNote = (message: string, title?: string) => void;
2725

@@ -513,6 +511,186 @@ describe("maybeRepairLegacyCronStore", () => {
513511
expectNoNoteContaining("Legacy cron job storage detected", "Cron");
514512
});
515513

514+
it("refuses a migration plan when the legacy source changes during confirmation", async () => {
515+
const storePath = await makeTempStorePath();
516+
await writeCronStore(storePath, [createLegacyCronJob()]);
517+
const changedJob = createLegacyCronJob({ jobId: "changed-job", name: "Changed job" });
518+
const prompter = {
519+
confirm: vi.fn(async () => {
520+
await writeCronStore(storePath, [changedJob]);
521+
return true;
522+
}),
523+
};
524+
525+
await maybeRepairLegacyCronStore({
526+
cfg: createCronConfig(storePath),
527+
options: {},
528+
prompter,
529+
});
530+
531+
expect(await readPersistedJobs(storePath)).toHaveLength(0);
532+
await expect(fs.readFile(storePath, "utf-8")).resolves.toContain("changed-job");
533+
await expect(fs.stat(`${storePath}.migrated`)).rejects.toMatchObject({ code: "ENOENT" });
534+
expectNoteContaining("changed while doctor was preparing", "Doctor warnings");
535+
expectNoNoteContaining("Cron store migrated to SQLite", "Doctor changes");
536+
537+
noteMock.mockClear();
538+
await maybeRepairLegacyCronStore({
539+
cfg: createCronConfig(storePath),
540+
options: {},
541+
prompter: makePrompter(true),
542+
});
543+
expect((await readPersistedJobs(storePath)).map((job) => job.id)).toEqual(["changed-job"]);
544+
await expect(fs.stat(storePath)).rejects.toMatchObject({ code: "ENOENT" });
545+
expectNoteContaining("Cron store migrated to SQLite", "Doctor changes");
546+
});
547+
548+
it("keeps a source that changes during an EXDEV copy and imports it on retry", async () => {
549+
const storePath = await makeTempStorePath();
550+
const archivePath = `${storePath}.migrated`;
551+
await writeCronStore(storePath, [createLegacyCronJob()]);
552+
553+
const renameSpy = mockExdevRename(storePath);
554+
const realCopyFile = fs.copyFile.bind(fs);
555+
const copyFileSpy = vi.spyOn(fs, "copyFile").mockImplementation(async (src, dest, mode) => {
556+
await realCopyFile(src, dest, mode);
557+
if (src === storePath) {
558+
await writeCronStore(storePath, [
559+
createLegacyCronJob({ jobId: "late-job", name: "Late job" }),
560+
]);
561+
}
562+
});
563+
564+
await withRestoredMocks([copyFileSpy, renameSpy], async () => {
565+
await maybeRepairLegacyCronStore({
566+
cfg: createCronConfig(storePath),
567+
options: {},
568+
prompter: makePrompter(true),
569+
});
570+
});
571+
572+
expect((await readPersistedJobs(storePath)).map((job) => job.id)).toEqual(["legacy-job"]);
573+
await expect(fs.readFile(storePath, "utf-8")).resolves.toContain("late-job");
574+
await expect(fs.stat(archivePath)).rejects.toMatchObject({ code: "ENOENT" });
575+
expectNoteContaining("changed during archival", "Doctor warnings");
576+
577+
noteMock.mockClear();
578+
await maybeRepairLegacyCronStore({
579+
cfg: createCronConfig(storePath),
580+
options: {},
581+
prompter: makePrompter(true),
582+
});
583+
expect((await readPersistedJobs(storePath)).map((job) => job.id)).toEqual([
584+
"legacy-job",
585+
"late-job",
586+
]);
587+
await expect(fs.stat(storePath)).rejects.toMatchObject({ code: "ENOENT" });
588+
await expect(fs.stat(archivePath)).resolves.toBeTruthy();
589+
});
590+
591+
it("restores an archived state sidecar when the primary archive fails", async () => {
592+
const storePath = await makeTempStorePath();
593+
const statePath = storePath.replace(/\.json$/, "-state.json");
594+
await writeCronStore(storePath, [createLegacyCronJob()]);
595+
await fs.writeFile(statePath, JSON.stringify({ version: 1, jobs: {} }), "utf-8");
596+
597+
const realRename = fs.rename.bind(fs);
598+
const renameSpy = vi.spyOn(fs, "rename").mockImplementation(async (oldPath, newPath) => {
599+
if (oldPath === storePath) {
600+
throw createFsError("EIO", "primary archive failed");
601+
}
602+
return await realRename(oldPath, newPath);
603+
});
604+
605+
await withRestoredMocks([renameSpy], async () => {
606+
await maybeRepairLegacyCronStore({
607+
cfg: createCronConfig(storePath),
608+
options: {},
609+
prompter: makePrompter(true),
610+
});
611+
});
612+
613+
await expect(fs.stat(storePath)).resolves.toBeTruthy();
614+
await expect(fs.stat(statePath)).resolves.toBeTruthy();
615+
await expect(fs.stat(`${statePath}.migrated`)).rejects.toMatchObject({ code: "ENOENT" });
616+
expectNoteContaining("EIO", "Doctor warnings");
617+
expectNoNoteContaining("Cron store migrated to SQLite", "Doctor changes");
618+
619+
noteMock.mockClear();
620+
await maybeRepairLegacyCronStore({
621+
cfg: createCronConfig(storePath),
622+
options: {},
623+
prompter: makePrompter(true),
624+
});
625+
await expect(fs.stat(storePath)).rejects.toMatchObject({ code: "ENOENT" });
626+
await expect(fs.stat(statePath)).rejects.toMatchObject({ code: "ENOENT" });
627+
await expect(fs.stat(`${statePath}.migrated`)).resolves.toBeTruthy();
628+
expectNoteContaining("Cron store migrated to SQLite", "Doctor changes");
629+
});
630+
631+
it("restores the primary source when a state sidecar is recreated during archival", async () => {
632+
const storePath = await makeTempStorePath();
633+
const statePath = storePath.replace(/\.json$/, "-state.json");
634+
await writeCronStore(storePath, [createLegacyCronJob()]);
635+
await fs.writeFile(statePath, JSON.stringify({ version: 1, jobs: {} }), "utf-8");
636+
637+
const realRename = fs.rename.bind(fs);
638+
const renameSpy = vi.spyOn(fs, "rename").mockImplementation(async (oldPath, newPath) => {
639+
if (oldPath === storePath) {
640+
await fs.writeFile(
641+
statePath,
642+
JSON.stringify({ version: 1, jobs: { "legacy-job": { state: { lastRunAtMs: 2 } } } }),
643+
"utf-8",
644+
);
645+
}
646+
return await realRename(oldPath, newPath);
647+
});
648+
649+
await withRestoredMocks([renameSpy], async () => {
650+
await maybeRepairLegacyCronStore({
651+
cfg: createCronConfig(storePath),
652+
options: {},
653+
prompter: makePrompter(true),
654+
});
655+
});
656+
657+
await expect(fs.stat(storePath)).resolves.toBeTruthy();
658+
await expect(fs.readFile(statePath, "utf-8")).resolves.toContain("lastRunAtMs");
659+
await expect(fs.stat(`${statePath}.migrated`)).resolves.toBeTruthy();
660+
expectNoteContaining("state appeared after", "Doctor warnings");
661+
expectNoteContaining("archive rollback failed", "Doctor warnings");
662+
expectNoNoteContaining("Cron store migrated to SQLite", "Doctor changes");
663+
});
664+
665+
it("reports a late state access failure without rejecting doctor", async () => {
666+
const storePath = await makeTempStorePath();
667+
const statePath = storePath.replace(/\.json$/, "-state.json");
668+
await writeCronStore(storePath, [createLegacyCronJob()]);
669+
670+
const realAccess = fs.access.bind(fs);
671+
let stateAccesses = 0;
672+
const accessSpy = vi.spyOn(fs, "access").mockImplementation(async (...args) => {
673+
if (args[0] === statePath && ++stateAccesses === 2) {
674+
throw createFsError("EIO", "state access failed");
675+
}
676+
return await realAccess(...args);
677+
});
678+
679+
await withRestoredMocks([accessSpy], async () => {
680+
await expect(
681+
maybeRepairLegacyCronStore({
682+
cfg: createCronConfig(storePath),
683+
options: {},
684+
prompter: makePrompter(true),
685+
}),
686+
).resolves.toBeUndefined();
687+
});
688+
689+
await expect(fs.stat(storePath)).resolves.toBeTruthy();
690+
expectNoteContaining("state access failed", "Doctor warnings");
691+
expectNoNoteContaining("Cron store migrated to SQLite", "Doctor changes");
692+
});
693+
516694
it("removes a partial copy and warns honestly when archiving fails", async () => {
517695
const storePath = await makeTempStorePath();
518696
const archivePath = `${storePath}.migrated`;
@@ -754,42 +932,42 @@ describe("maybeRepairLegacyCronStore", () => {
754932
},
755933
);
756934

757-
it("reconciles a shipped random-id migration before archiving its leftover source", async () => {
935+
it("does not resurrect a migrated job removed before an archive retry", async () => {
758936
const storePath = await makeTempStorePath();
759-
const legacyJob = createLegacyCronJob({
760-
id: undefined,
761-
jobId: undefined,
762-
description: "",
763-
sessionKey: "",
764-
createdAtMs: undefined,
765-
updatedAtMs: undefined,
766-
schedule: { kind: "every", everyMs: 60_000 },
767-
payload: { kind: "agentTurn", message: " Morning brief ", thinking: null },
768-
});
769-
const previouslyMigrated = structuredClone(legacyJob);
770-
previouslyMigrated.updatedAtMs = 1_000;
771-
normalizeStoredCronJobs([previouslyMigrated]);
772-
migrateLegacyNotifyFallback({
773-
jobs: [previouslyMigrated],
774-
legacyWebhook: "https://old.example.invalid/cron-finished",
775-
});
776-
migrateLegacyDreamingPayloadShape([previouslyMigrated]);
777-
delete (previouslyMigrated.payload as Record<string, unknown>).thinking;
778-
previouslyMigrated.id = "cron-11111111-2222-4333-8444-555555555555";
779-
await writeCurrentCronStore(storePath, [previouslyMigrated]);
780-
await writeCronStore(storePath, [legacyJob]);
937+
const archivePath = `${storePath}.migrated`;
938+
await writeCronStore(storePath, [createLegacyCronJob({ id: undefined, jobId: undefined })]);
781939

940+
const renameSpy = mockExdevRename(storePath);
941+
const realUnlink = fs.unlink.bind(fs);
942+
const unlinkSpy = vi.spyOn(fs, "unlink").mockImplementation(async (target) => {
943+
if (target === storePath) {
944+
throw createFsError("EBUSY", "resource busy, unlink");
945+
}
946+
return await realUnlink(target);
947+
});
948+
949+
await withRestoredMocks([unlinkSpy, renameSpy], async () => {
950+
await maybeRepairLegacyCronStore({
951+
cfg: createCronConfig(storePath),
952+
options: {},
953+
prompter: makePrompter(true),
954+
});
955+
expectNoteContaining("EBUSY", "Doctor warnings");
956+
});
957+
expect(await readPersistedJobs(storePath)).toHaveLength(1);
958+
959+
// Simulate runtime-owned one-shot deletion after SQLite import but before cleanup retry.
960+
await writeCurrentCronStore(storePath, []);
961+
noteMock.mockClear();
782962
await maybeRepairLegacyCronStore({
783963
cfg: createCronConfig(storePath),
784964
options: {},
785965
prompter: makePrompter(true),
786966
});
787967

788-
const jobs = await readPersistedJobs(storePath);
789-
expect(jobs).toHaveLength(1);
790-
expect(jobs[0]?.id).toBe("cron-11111111-2222-4333-8444-555555555555");
968+
expect(await readPersistedJobs(storePath)).toHaveLength(0);
791969
await expect(fs.stat(storePath)).rejects.toMatchObject({ code: "ENOENT" });
792-
await expect(fs.stat(`${storePath}.migrated`)).resolves.toBeTruthy();
970+
await expect(fs.stat(archivePath)).resolves.toBeTruthy();
793971
expectNoteContaining("Cron store migrated to SQLite", "Doctor changes");
794972
});
795973

@@ -928,6 +1106,35 @@ describe("maybeRepairLegacyCronStore", () => {
9281106
expectNoteContaining("Cron run logs migrated to SQLite", "Doctor changes");
9291107
});
9301108

1109+
it("does not report store normalization when run-log migration fails", async () => {
1110+
const storePath = await makeTempStorePath();
1111+
await writeCurrentCronStore(storePath, [createCurrentCronJob()]);
1112+
const runLogPath = path.join(path.dirname(storePath), "runs", "sqlite-job.jsonl");
1113+
await fs.mkdir(path.dirname(runLogPath), { recursive: true });
1114+
await fs.writeFile(runLogPath, "{}\n", "utf-8");
1115+
1116+
const realReadFileSync = fsSync.readFileSync.bind(fsSync);
1117+
const readSpy = vi.spyOn(fsSync, "readFileSync").mockImplementation((filePath, options) => {
1118+
if (filePath === runLogPath) {
1119+
throw createFsError("EIO", "run-log read failed");
1120+
}
1121+
return realReadFileSync(filePath as never, options as never) as never;
1122+
});
1123+
1124+
await withRestoredMocks([readSpy], async () => {
1125+
await maybeRepairLegacyCronStore({
1126+
cfg: createCronConfig(storePath),
1127+
options: {},
1128+
prompter: makePrompter(true),
1129+
});
1130+
});
1131+
1132+
await expect(fs.stat(runLogPath)).resolves.toBeTruthy();
1133+
expectNoteContaining("run-log read failed", "Doctor warnings");
1134+
expectNoNoteContaining("Cron store normalized", "Doctor changes");
1135+
expectNoNoteContaining("Cron run logs migrated", "Doctor changes");
1136+
});
1137+
9311138
it("does not claim legacy store detected when only non-legacy issues exist (#92683)", async () => {
9321139
const storePath = await makeTempStorePath();
9331140
await writeCurrentCronStore(storePath, [

0 commit comments

Comments
 (0)