Fix delete-agent cron rollback data-loss path

src/features/agents/operations/deleteAgentOperation.ts

@@ -1,6 +1,10 @@
 import type { GatewayClient } from "@/lib/gateway/GatewayClient";
 import { fetchJson as defaultFetchJson } from "@/lib/http";
-import { removeCronJobsForAgent } from "@/lib/cron/types";
+import {
+  removeCronJobsForAgentWithBackup,
+  restoreCronJobs,
+  type CronJobRestoreInput,
+} from "@/lib/cron/types";
 import { deleteGatewayAgent } from "@/lib/gateway/agentConfig";
 
 type FetchJson = typeof defaultFetchJson;
@@ -19,7 +23,8 @@ export type RestoreAgentStateResult = {
 type DeleteAgentTransactionDeps = {
   trashAgentState: (agentId: string) => Promise<TrashAgentStateResult>;
   restoreAgentState: (agentId: string, trashDir: string) => Promise<RestoreAgentStateResult>;
-  removeCronJobsForAgent: (agentId: string) => Promise<void>;
+  removeCronJobsForAgentWithBackup: (agentId: string) => Promise<CronJobRestoreInput[]>;
+  restoreCronJobs: (jobs: CronJobRestoreInput[]) => Promise<void>;
   deleteGatewayAgent: (agentId: string) => Promise<void>;
   logError?: (message: string, error: unknown) => void;
 };
@@ -39,12 +44,20 @@ const runDeleteFlow = async (
   }
 
   const trashed = await deps.trashAgentState(trimmedAgentId);
+  let removedCronJobs: CronJobRestoreInput[] = [];
 
   try {
-    await deps.removeCronJobsForAgent(trimmedAgentId);
+    removedCronJobs = await deps.removeCronJobsForAgentWithBackup(trimmedAgentId);
     await deps.deleteGatewayAgent(trimmedAgentId);
     return { trashed, restored: null };
   } catch (err) {
+    if (removedCronJobs.length > 0) {
+      try {
+        await deps.restoreCronJobs(removedCronJobs);
+      } catch (restoreCronErr) {
+        deps.logError?.("Failed to restore removed cron jobs.", restoreCronErr);
+      }
+    }
     if (trashed.moved.length > 0) {
       try {
         await deps.restoreAgentState(trimmedAgentId, trashed.trashDir);
@@ -89,8 +102,11 @@ export const deleteAgentViaStudio = async (params: {
         );
         return result;
       },
-      removeCronJobsForAgent: async (agentId) => {
-        await removeCronJobsForAgent(params.client, agentId);
+      removeCronJobsForAgentWithBackup: async (agentId) => {
+        return await removeCronJobsForAgentWithBackup(params.client, agentId);
+      },
+      restoreCronJobs: async (jobs) => {
+        await restoreCronJobs(params.client, jobs);
       },
       deleteGatewayAgent: async (agentId) => {
         await deleteGatewayAgent({ client: params.client, agentId });

src/lib/cron/types.ts

@@ -44,7 +44,10 @@ export type CronJobSummary = {
   id: string;
   name: string;
   agentId?: string;
+  sessionKey?: string;
+  description?: string;
   enabled: boolean;
+  deleteAfterRun?: boolean;
   updatedAtMs: number;
   schedule: CronSchedule;
   sessionTarget: CronSessionTarget;
@@ -64,6 +67,7 @@ export const sortCronJobsByUpdatedAt = (jobs: CronJobSummary[]) =>
 export type CronJobCreateInput = {
   name: string;
   agentId: string;
+  sessionKey?: string;
   description?: string;
   enabled?: boolean;
   deleteAfterRun?: boolean;
@@ -137,6 +141,20 @@ export type CronRunResult =
 
 export type CronRemoveResult = { ok: true; removed: boolean } | { ok: false; removed: false };
 
+export type CronJobRestoreInput = {
+  name: string;
+  agentId: string;
+  sessionKey?: string;
+  description?: string;
+  enabled: boolean;
+  deleteAfterRun?: boolean;
+  schedule: CronSchedule;
+  sessionTarget: CronSessionTarget;
+  wakeMode: CronWakeMode;
+  payload: CronPayload;
+  delivery?: CronDelivery;
+};
+
 const resolveJobId = (jobId: string): string => {
   const trimmed = jobId.trim();
   if (!trimmed) {
@@ -202,19 +220,74 @@ export const createCronJob = async (
   });
 };
 
-export const removeCronJobsForAgent = async (client: GatewayClient, agentId: string): Promise<number> => {
+const toCronJobRestoreInput = (job: CronJobSummary, agentId: string): CronJobRestoreInput => ({
+  name: job.name,
+  agentId,
+  sessionKey: job.sessionKey,
+  description: job.description,
+  enabled: job.enabled,
+  deleteAfterRun: job.deleteAfterRun,
+  schedule: job.schedule,
+  sessionTarget: job.sessionTarget,
+  wakeMode: job.wakeMode,
+  payload: job.payload,
+  delivery: job.delivery,
+});
+
+const restoreRemovedJobsBestEffort = async (
+  client: GatewayClient,
+  removedJobs: CronJobRestoreInput[]
+): Promise<void> => {
+  if (removedJobs.length === 0) return;
+  try {
+    await restoreCronJobs(client, removedJobs);
+  } catch (restoreErr) {
+    console.error("Failed to restore cron jobs after partial deletion failure.", restoreErr);
+  }
+};
+
+export const restoreCronJobs = async (
+  client: GatewayClient,
+  jobs: CronJobRestoreInput[]
+): Promise<void> => {
+  for (const job of jobs) {
+    try {
+      await createCronJob(client, job);
+    } catch (err) {
+      const message = err instanceof Error ? err.message : String(err);
+      throw new Error(`Failed to restore cron job "${job.name}" (${job.agentId}): ${message}`);
+    }
+  }
+};
+
+export const removeCronJobsForAgentWithBackup = async (
+  client: GatewayClient,
+  agentId: string
+): Promise<CronJobRestoreInput[]> => {
   const id = resolveAgentId(agentId);
   const result = await listCronJobs(client, { includeDisabled: true });
   const jobs = result.jobs.filter((job) => job.agentId?.trim() === id);
-  let removed = 0;
+  const removedJobs: CronJobRestoreInput[] = [];
   for (const job of jobs) {
-    const removeResult = await removeCronJob(client, job.id);
+    let removeResult: CronRemoveResult;
+    try {
+      removeResult = await removeCronJob(client, job.id);
+    } catch (err) {
+      await restoreRemovedJobsBestEffort(client, removedJobs);
+      throw err;
+    }
     if (!removeResult.ok) {
+      await restoreRemovedJobsBestEffort(client, removedJobs);
       throw new Error(`Failed to delete cron job "${job.name}" (${job.id}).`);
     }
     if (removeResult.removed) {
-      removed += 1;
+      removedJobs.push(toCronJobRestoreInput(job, id));
     }
   }
-  return removed;
+  return removedJobs;
+};
+
+export const removeCronJobsForAgent = async (client: GatewayClient, agentId: string): Promise<number> => {
+  const removedJobs = await removeCronJobsForAgentWithBackup(client, agentId);
+  return removedJobs.length;
 };

tests/unit/cronGatewayClient.test.ts

@@ -5,10 +5,31 @@ import {
   listCronJobs,
   removeCronJob,
   removeCronJobsForAgent,
+  removeCronJobsForAgentWithBackup,
+  restoreCronJobs,
   runCronJobNow,
+  type CronJobSummary,
 } from "@/lib/cron/types";
 import type { GatewayClient } from "@/lib/gateway/GatewayClient";
 
+const createListedJob = (params: {
+  id: string;
+  name: string;
+  agentId?: string;
+  updatedAtMs?: number;
+}): CronJobSummary => ({
+  id: params.id,
+  name: params.name,
+  agentId: params.agentId,
+  enabled: true,
+  updatedAtMs: params.updatedAtMs ?? 1_700_000_000_000,
+  schedule: { kind: "every", everyMs: 60_000 },
+  sessionTarget: "isolated",
+  wakeMode: "now",
+  payload: { kind: "agentTurn", message: "Run checks." },
+  state: {},
+});
+
 describe("cron gateway client", () => {
   it("lists_jobs_via_cron_list_include_disabled_true", async () => {
     const client = {
@@ -55,9 +76,9 @@ describe("cron gateway client", () => {
         if (method === "cron.list") {
           return {
             jobs: [
-              { id: "job-1", name: "Job 1", agentId: "agent-1" },
-              { id: "job-2", name: "Job 2", agentId: "agent-2" },
-              { id: "job-3", name: "Job 3", agentId: "agent-1" },
+              createListedJob({ id: "job-1", name: "Job 1", agentId: "agent-1" }),
+              createListedJob({ id: "job-2", name: "Job 2", agentId: "agent-2" }),
+              createListedJob({ id: "job-3", name: "Job 3", agentId: "agent-1" }),
             ],
           };
         }
@@ -87,7 +108,7 @@ describe("cron gateway client", () => {
       call: vi.fn(async (method: string) => {
         if (method === "cron.list") {
           return {
-            jobs: [{ id: "job-1", name: "Job 1", agentId: "agent-1" }],
+            jobs: [createListedJob({ id: "job-1", name: "Job 1", agentId: "agent-1" })],
           };
         }
         if (method === "cron.remove") {
@@ -102,6 +123,157 @@ describe("cron gateway client", () => {
     );
   });
 
+  it("returns_restore_inputs_when_removing_jobs_with_backup", async () => {
+    const client = {
+      call: vi.fn(async (method: string, payload: { id?: string }) => {
+        if (method === "cron.list") {
+          return {
+            jobs: [
+              createListedJob({ id: "job-1", name: "Job 1", agentId: "agent-1" }),
+              createListedJob({ id: "job-2", name: "Job 2", agentId: "agent-2" }),
+              createListedJob({ id: "job-3", name: "Job 3", agentId: "agent-1" }),
+            ],
+          };
+        }
+        if (method === "cron.remove") {
+          return { ok: true, removed: payload.id !== "job-3" };
+        }
+        throw new Error(`Unexpected method: ${method}`);
+      }),
+    } as unknown as GatewayClient;
+
+    await expect(removeCronJobsForAgentWithBackup(client, "agent-1")).resolves.toEqual([
+      {
+        name: "Job 1",
+        agentId: "agent-1",
+        sessionKey: undefined,
+        description: undefined,
+        enabled: true,
+        deleteAfterRun: undefined,
+        schedule: { kind: "every", everyMs: 60_000 },
+        sessionTarget: "isolated",
+        wakeMode: "now",
+        payload: { kind: "agentTurn", message: "Run checks." },
+        delivery: undefined,
+      },
+    ]);
+  });
+
+  it("restores_removed_jobs_when_backup_remove_fails_midway", async () => {
+    const client = {
+      call: vi.fn(async (method: string, payload: { id?: string; name?: string }) => {
+        if (method === "cron.list") {
+          return {
+            jobs: [
+              createListedJob({ id: "job-1", name: "Job 1", agentId: "agent-1" }),
+              createListedJob({ id: "job-2", name: "Job 2", agentId: "agent-1" }),
+            ],
+          };
+        }
+        if (method === "cron.remove") {
+          if (payload.id === "job-1") return { ok: true, removed: true };
+          return { ok: false, removed: false };
+        }
+        if (method === "cron.add") {
+          return { id: "restored-job-1", name: payload.name };
+        }
+        throw new Error(`Unexpected method: ${method}`);
+      }),
+    } as unknown as GatewayClient;
+
+    await expect(removeCronJobsForAgentWithBackup(client, "agent-1")).rejects.toThrow(
+      'Failed to delete cron job "Job 2" (job-2).'
+    );
+
+    expect(client.call).toHaveBeenCalledWith("cron.add", {
+      name: "Job 1",
+      agentId: "agent-1",
+      sessionKey: undefined,
+      description: undefined,
+      enabled: true,
+      deleteAfterRun: undefined,
+      schedule: { kind: "every", everyMs: 60_000 },
+      sessionTarget: "isolated",
+      wakeMode: "now",
+      payload: { kind: "agentTurn", message: "Run checks." },
+      delivery: undefined,
+    });
+  });
+
+  it("restores_removed_jobs_when_remove_call_throws_midway", async () => {
+    const thrown = new Error("network interrupted");
+    const client = {
+      call: vi.fn(async (method: string, payload: { id?: string; name?: string }) => {
+        if (method === "cron.list") {
+          return {
+            jobs: [
+              createListedJob({ id: "job-1", name: "Job 1", agentId: "agent-1" }),
+              createListedJob({ id: "job-2", name: "Job 2", agentId: "agent-1" }),
+            ],
+          };
+        }
+        if (method === "cron.remove") {
+          if (payload.id === "job-1") return { ok: true, removed: true };
+          throw thrown;
+        }
+        if (method === "cron.add") {
+          return { id: "restored-job-1", name: payload.name };
+        }
+        throw new Error(`Unexpected method: ${method}`);
+      }),
+    } as unknown as GatewayClient;
+
+    await expect(removeCronJobsForAgentWithBackup(client, "agent-1")).rejects.toBe(thrown);
+
+    expect(client.call).toHaveBeenCalledWith("cron.add", {
+      name: "Job 1",
+      agentId: "agent-1",
+      sessionKey: undefined,
+      description: undefined,
+      enabled: true,
+      deleteAfterRun: undefined,
+      schedule: { kind: "every", everyMs: 60_000 },
+      sessionTarget: "isolated",
+      wakeMode: "now",
+      payload: { kind: "agentTurn", message: "Run checks." },
+      delivery: undefined,
+    });
+  });
+
+  it("throws_actionable_error_when_restore_fails", async () => {
+    const client = {
+      call: vi.fn(async (_method: string, payload: { name?: string }) => {
+        if (payload.name === "Job 2") {
+          throw new Error("cron.add failed");
+        }
+        return { id: "job-restored", name: payload.name };
+      }),
+    } as unknown as GatewayClient;
+
+    await expect(
+      restoreCronJobs(client, [
+        {
+          name: "Job 1",
+          agentId: "agent-1",
+          enabled: true,
+          schedule: { kind: "every", everyMs: 60_000 },
+          sessionTarget: "isolated",
+          wakeMode: "now",
+          payload: { kind: "agentTurn", message: "Run checks." },
+        },
+        {
+          name: "Job 2",
+          agentId: "agent-1",
+          enabled: true,
+          schedule: { kind: "every", everyMs: 120_000 },
+          sessionTarget: "isolated",
+          wakeMode: "now",
+          payload: { kind: "agentTurn", message: "Run checks again." },
+        },
+      ])
+    ).rejects.toThrow('Failed to restore cron job "Job 2" (agent-1): cron.add failed');
+  });
+
   it("creates_job_via_cron_add", async () => {
     const client = {
       call: vi.fn(async () => ({ id: "job-1", name: "Morning brief" })),

tests/unit/deleteAgentOperation.test.ts

@@ -1,12 +1,20 @@
 import { describe, expect, it, vi, beforeEach } from "vitest";
 
-import { removeCronJobsForAgent } from "@/lib/cron/types";
+import {
+  removeCronJobsForAgentWithBackup,
+  restoreCronJobs,
+  type CronJobRestoreInput,
+} from "@/lib/cron/types";
 import { deleteGatewayAgent } from "@/lib/gateway/agentConfig";
 import { deleteAgentViaStudio } from "@/features/agents/operations/deleteAgentOperation";
 
 vi.mock("@/lib/cron/types", async () => {
   const actual = await vi.importActual<typeof import("@/lib/cron/types")>("@/lib/cron/types");
-  return { ...actual, removeCronJobsForAgent: vi.fn() };
+  return {
+    ...actual,
+    removeCronJobsForAgentWithBackup: vi.fn(),
+    restoreCronJobs: vi.fn(),
+  };
 });
 
 vi.mock("@/lib/gateway/agentConfig", async () => {
@@ -27,12 +35,24 @@ const createTrashResult = (overrides?: {
   ...(overrides ?? {}),
 });
 
+const createCronRestoreInput = (name = "Job 1", agentId = "agent-1"): CronJobRestoreInput => ({
+  name,
+  agentId,
+  enabled: true,
+  schedule: { kind: "every", everyMs: 60_000 },
+  sessionTarget: "isolated",
+  wakeMode: "now",
+  payload: { kind: "agentTurn", message: "Run checks." },
+});
+
 describe("delete agent via studio operation", () => {
-  const mockedRemoveCronJobsForAgent = vi.mocked(removeCronJobsForAgent);
+  const mockedRemoveCronJobsForAgentWithBackup = vi.mocked(removeCronJobsForAgentWithBackup);
+  const mockedRestoreCronJobs = vi.mocked(restoreCronJobs);
   const mockedDeleteGatewayAgent = vi.mocked(deleteGatewayAgent);
 
   beforeEach(() => {
-    mockedRemoveCronJobsForAgent.mockReset();
+    mockedRemoveCronJobsForAgentWithBackup.mockReset();
+    mockedRestoreCronJobs.mockReset();
     mockedDeleteGatewayAgent.mockReset();
   });
 
@@ -46,9 +66,12 @@ describe("delete agent via studio operation", () => {
       throw new Error("Unexpected fetchJson call");
     });
 
-    mockedRemoveCronJobsForAgent.mockImplementation(async () => {
+    mockedRemoveCronJobsForAgentWithBackup.mockImplementation(async () => {
       calls.push("removeCron");
-      return 0;
+      return [];
+    });
+    mockedRestoreCronJobs.mockImplementation(async () => {
+      calls.push("restoreCron");
     });
     mockedDeleteGatewayAgent.mockImplementation(async () => {
       calls.push("deleteGatewayAgent");
@@ -85,10 +108,13 @@ describe("delete agent via studio operation", () => {
       throw new Error("Unexpected fetchJson call");
     });
 
-    mockedRemoveCronJobsForAgent.mockImplementation(async () => {
+    mockedRemoveCronJobsForAgentWithBackup.mockImplementation(async () => {
       calls.push("removeCron");
       throw originalErr;
     });
+    mockedRestoreCronJobs.mockImplementation(async () => {
+      calls.push("restoreCron");
+    });
     mockedDeleteGatewayAgent.mockImplementation(async () => {
       calls.push("deleteGatewayAgent");
       return { removed: true, removedBindings: 0 };
@@ -99,12 +125,14 @@ describe("delete agent via studio operation", () => {
     ).rejects.toBe(originalErr);
 
     expect(calls).toEqual(["trash", "removeCron", "restore:agent-1:/tmp/trash-2"]);
+    expect(mockedRestoreCronJobs).not.toHaveBeenCalled();
     expect(mockedDeleteGatewayAgent).not.toHaveBeenCalled();
   });
 
-  it("attempts_restore_when_gateway_delete_fails_and_trash_moved_paths", async () => {
+  it("attempts_cron_restore_then_state_restore_when_gateway_delete_fails_and_trash_moved_paths", async () => {
     const calls: string[] = [];
     const originalErr = new Error("boom");
+    const backups = [createCronRestoreInput("Job X", "agent-1")];
 
     const fetchJson: FetchJson = vi.fn(async (_input, init) => {
       if (init?.method === "POST") {
@@ -123,9 +151,12 @@ describe("delete agent via studio operation", () => {
       throw new Error("Unexpected fetchJson call");
     });
 
-    mockedRemoveCronJobsForAgent.mockImplementation(async () => {
+    mockedRemoveCronJobsForAgentWithBackup.mockImplementation(async () => {
       calls.push("removeCron");
-      return 0;
+      return backups;
+    });
+    mockedRestoreCronJobs.mockImplementation(async () => {
+      calls.push("restoreCron");
     });
     mockedDeleteGatewayAgent.mockImplementation(async () => {
       calls.push("deleteGatewayAgent");
@@ -140,8 +171,10 @@ describe("delete agent via studio operation", () => {
       "trash",
       "removeCron",
       "deleteGatewayAgent",
+      "restoreCron",
       "restore:agent-1:/tmp/trash-3",
     ]);
+    expect(mockedRestoreCronJobs).toHaveBeenCalledWith(expect.anything(), backups);
   });
 
   it("does_not_restore_when_trash_moved_is_empty", async () => {
@@ -160,9 +193,10 @@ describe("delete agent via studio operation", () => {
       throw new Error("Unexpected fetchJson call");
     });
 
-    mockedRemoveCronJobsForAgent.mockImplementation(async () => {
+    mockedRemoveCronJobsForAgentWithBackup.mockImplementation(async () => {
       throw originalErr;
     });
+    mockedRestoreCronJobs.mockResolvedValue(undefined);
     mockedDeleteGatewayAgent.mockImplementation(async () => {
       return { removed: true, removedBindings: 0 };
     });
@@ -173,12 +207,15 @@ describe("delete agent via studio operation", () => {
 
     expect(methods).toEqual(["POST"]);
     expect(mockedDeleteGatewayAgent).not.toHaveBeenCalled();
+    expect(mockedRestoreCronJobs).not.toHaveBeenCalled();
   });
 
-  it("logs_restore_failure_and_still_throws_original_error", async () => {
+  it("logs_cron_and_state_restore_failures_and_still_throws_original_error", async () => {
     const originalErr = new Error("boom");
+    const cronRestoreErr = new Error("cron-restore-failed");
     const restoreErr = new Error("restore-failed");
     const logError = vi.fn();
+    const backups = [createCronRestoreInput("Job Z", "agent-1")];
 
     const fetchJson: FetchJson = vi.fn(async (_input, init) => {
       if (init?.method === "POST") {
@@ -195,8 +232,11 @@ describe("delete agent via studio operation", () => {
       throw new Error("Unexpected fetchJson call");
     });
 
-    mockedRemoveCronJobsForAgent.mockImplementation(async () => {
-      return 0;
+    mockedRemoveCronJobsForAgentWithBackup.mockImplementation(async () => {
+      return backups;
+    });
+    mockedRestoreCronJobs.mockImplementation(async () => {
+      throw cronRestoreErr;
     });
     mockedDeleteGatewayAgent.mockImplementation(async () => {
       throw originalErr;
@@ -211,8 +251,13 @@ describe("delete agent via studio operation", () => {
       })
     ).rejects.toBe(originalErr);
 
-    expect(logError).toHaveBeenCalledTimes(1);
-    expect(logError).toHaveBeenCalledWith("Failed to restore trashed agent state.", restoreErr);
+    expect(logError).toHaveBeenCalledTimes(2);
+    expect(logError).toHaveBeenNthCalledWith(
+      1,
+      "Failed to restore removed cron jobs.",
+      cronRestoreErr
+    );
+    expect(logError).toHaveBeenNthCalledWith(2, "Failed to restore trashed agent state.", restoreErr);
   });
 
   it("fails_fast_when_agent_id_is_missing", async () => {
@@ -225,7 +270,8 @@ describe("delete agent via studio operation", () => {
     ).rejects.toThrow("Agent id is required.");
 
     expect(fetchJson).not.toHaveBeenCalled();
-    expect(mockedRemoveCronJobsForAgent).not.toHaveBeenCalled();
+    expect(mockedRemoveCronJobsForAgentWithBackup).not.toHaveBeenCalled();
+    expect(mockedRestoreCronJobs).not.toHaveBeenCalled();
     expect(mockedDeleteGatewayAgent).not.toHaveBeenCalled();
   });
 });