diff --git a/src/features/agents/operations/deleteAgentOperation.ts b/src/features/agents/operations/deleteAgentOperation.ts index d36bd7c..1f29d22 100644 --- a/src/features/agents/operations/deleteAgentOperation.ts +++ b/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; restoreAgentState: (agentId: string, trashDir: string) => Promise; - removeCronJobsForAgent: (agentId: string) => Promise; + removeCronJobsForAgentWithBackup: (agentId: string) => Promise; + restoreCronJobs: (jobs: CronJobRestoreInput[]) => Promise; deleteGatewayAgent: (agentId: string) => Promise; 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 }); diff --git a/src/lib/cron/types.ts b/src/lib/cron/types.ts index deb5c4e..c797e36 100644 --- a/src/lib/cron/types.ts +++ b/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 => { +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 => { + 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 => { + 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 => { 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 => { + const removedJobs = await removeCronJobsForAgentWithBackup(client, agentId); + return removedJobs.length; }; diff --git a/tests/unit/cronGatewayClient.test.ts b/tests/unit/cronGatewayClient.test.ts index 59b1f67..3fc1e4b 100644 --- a/tests/unit/cronGatewayClient.test.ts +++ b/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" })), diff --git a/tests/unit/deleteAgentOperation.test.ts b/tests/unit/deleteAgentOperation.test.ts index 3932b9f..5424d1d 100644 --- a/tests/unit/deleteAgentOperation.test.ts +++ b/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("@/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(); }); });