From 6082bceee6de8fecd984d7852163d1754b8c543a Mon Sep 17 00:00:00 2001 From: Kris Date: Mon, 29 Jun 2026 11:50:42 +0530 Subject: [PATCH] fix(cron): close three MED cron-job defects 1. once-rearm on edit: editing any field of a finished one-time job reset completedOnce, silently resurrecting it. Now only a SCHEDULE edit (scheduleType/runAt/interval/daily/weekly) re-arms a completed once job; cosmetic edits (rename/notes) leave completedOnce intact. 2. update-validation gap: CronJobUpdateSchema = .partial() drops the cross-field superRefine, so a PUT switching scheduleType without its dependent field produced a dead enabled job (nextRunAt:null). updateJob now re-validates the MERGED job against the full CronJobSchema and throws 400 on inconsistency, leaving the stored job untouched. 3. concurrency-skip silent starvation: skip_if_same_agent_running advanced the schedule but wrote no run record, so a perpetually-skipped job had empty history. Now records a 'skipped' run (new CronJobRunStatus) + lastStatus. Tests updated/added in cron-service.test.ts (37 pass): once non-schedule edit preserves completedOnce, schedule edit re-arms, inconsistent partial update is rejected with the stored job untouched, and the skip path records a skipped run. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01PmvZR12aX2v8K7YhqxPUAU --- src/cron/cron-service.ts | 56 ++++++++++++++++++++++++++++++++++++--- src/types/cron.ts | 2 +- test/cron-service.test.ts | 55 ++++++++++++++++++++++++++++++++------ 3 files changed, 100 insertions(+), 13 deletions(-) diff --git a/src/cron/cron-service.ts b/src/cron/cron-service.ts index 2e387091..e2b19464 100644 --- a/src/cron/cron-service.ts +++ b/src/cron/cron-service.ts @@ -12,7 +12,8 @@ import { readFile } from 'node:fs/promises'; import { statSync, realpathSync } from 'node:fs'; import { Session } from '../session.js'; import { SseEvent } from '../web/sse-events.js'; -import { getErrorMessage } from '../types/api.js'; +import { CronJobSchema } from '../web/schemas.js'; +import { getErrorMessage, createErrorResponse, ApiErrorCode } from '../types/api.js'; import { MAX_CONCURRENT_SESSIONS } from '../config/map-limits.js'; import { CRON_READY_MAX_ATTEMPTS, CRON_READY_SETTLE_MS } from '../config/server-timing.js'; import { isBlockedAttachmentPath, loadAttachmentGuardConfig } from '../config/attachment-guard.js'; @@ -97,17 +98,42 @@ export class CronService { const existing = this.getJob(id); if (!existing) return null; const now = Date.now(); + + // A completed one-time job is only re-armed when the SCHEDULE itself is + // edited — otherwise a cosmetic edit (rename, notes) would silently + // resurrect a job that already fired and run it again. + const scheduleTouched = + patch.scheduleType !== undefined || + patch.runAt !== undefined || + patch.intervalMinutes !== undefined || + patch.dailyTime !== undefined || + patch.weeklyTime !== undefined || + patch.weeklyDays !== undefined; + const reArm = existing.scheduleType !== 'once' || !existing.completedOnce || scheduleTouched; + const updated: CronJob = { ...existing, ...patch, id: existing.id, createdAt: existing.createdAt, updatedAt: now, - // Editing a job re-arms it: clear the one-time completion + dup-guard so a - // changed schedule can fire again. - completedOnce: false, + completedOnce: reArm ? false : existing.completedOnce, lastDueKey: null, }; + + // The PUT schema is `.partial()`, so its cross-field rules don't run on a + // partial body. Re-validate the MERGED job against the full schema so a + // partial edit can't leave an enabled job with an inconsistent schedule + // (e.g. switching to `once` without a `runAt` → a dead `nextRunAt:null`). + const check = CronJobSchema.safeParse(updated); + if (!check.success) { + const msg = check.error.issues[0]?.message ?? 'Invalid cron job update'; + throw Object.assign(new Error(msg), { + statusCode: 400, + body: createErrorResponse(ApiErrorCode.INVALID_INPUT, msg), + }); + } + updated.nextRunAt = updated.enabled ? computeNextRunAt(updated, now) : null; this.store.setCronJob(updated.id, updated); this.broadcastListChanged(); @@ -162,6 +188,9 @@ export class CronService { // Optional concurrency policy for AUTOMATIC runs. if (job.concurrencyPolicy === 'skip_if_same_agent_running' && this.countActiveAgents(job.agentType) > 0) { job.lastDueKey = key; + // Record the skip so the job's run history isn't silently empty when it + // keeps getting skipped (otherwise it looks like the job never ran). + this.recordSkippedRun(job); this.advanceAfterFire(job, now); continue; } @@ -371,6 +400,25 @@ export class CronService { return run; } + private recordSkippedRun(job: CronJob): void { + const now = Date.now(); + const run: CronJobRun = { + id: uuidv4(), + cronJobId: job.id, + sessionId: null, + sessionName: null, + startedAt: now, + finishedAt: now, + status: 'skipped', + errorMessage: `Skipped: a ${job.agentType} agent is already running (concurrency policy)`, + triggerType: 'scheduled', + createdSessionUrl: null, + }; + this.store.setCronJobRun(run.id, run); + this.deps.broadcast(SseEvent.CronRunCreated, run); + this.updateJobLastStatus(job.id, 'skipped'); + } + private updateJobLastStatus(jobId: string, status: CronJobRunStatus): void { const fresh = this.store.getCronJob(jobId); if (!fresh) return; diff --git a/src/types/cron.ts b/src/types/cron.ts index 9e7de29f..18e96a99 100644 --- a/src/types/cron.ts +++ b/src/types/cron.ts @@ -22,7 +22,7 @@ export type PromptMode = 'inline_text' | 'prompt_file_path'; export type InputMode = 'paste' | 'typed'; /** Lifecycle status of a single job execution. */ -export type CronJobRunStatus = 'created' | 'session_started' | 'prompt_sent' | 'failed'; +export type CronJobRunStatus = 'created' | 'session_started' | 'prompt_sent' | 'failed' | 'skipped'; /** What triggered a run. */ export type TriggerType = 'scheduled' | 'manual_run_now'; diff --git a/test/cron-service.test.ts b/test/cron-service.test.ts index c529488c..97f1be6e 100644 --- a/test/cron-service.test.ts +++ b/test/cron-service.test.ts @@ -117,19 +117,54 @@ describe('CronService', () => { }); describe('updateJob', () => { - it('re-arms the dup-guard and once-completion flags', () => { - const job = svc.service.createJob( - mkInput({ scheduleType: 'once', runAt: Date.now() + 1000, intervalMinutes: undefined }) - ); + it('clears the dup-guard and preserves createdAt', () => { + const job = svc.service.createJob(mkInput({ intervalMinutes: 10 })); job.lastDueKey = 'stale'; - job.completedOnce = true; svc.store.setCronJob(job.id, job); const updated = svc.service.updateJob(job.id, { name: 'renamed' }); expect(updated!.name).toBe('renamed'); expect(updated!.lastDueKey).toBeNull(); - expect(updated!.completedOnce).toBe(false); expect(updated!.createdAt).toBe(job.createdAt); }); + + it('does NOT re-fire a completed once-job when editing a non-schedule field', async () => { + const job = svc.service.createJob( + mkInput({ scheduleType: 'once', runAt: Date.now() - 1000, intervalMinutes: undefined }) + ); + await svc.service.tickDueJobs(Date.now()); + await flush(); + expect(svc.service.getJob(job.id)!.completedOnce).toBe(true); + + const updated = svc.service.updateJob(job.id, { name: 'renamed' }); + expect(updated!.name).toBe('renamed'); + // Cosmetic edit must not resurrect a fired one-time job. + expect(updated!.completedOnce).toBe(true); + expect(updated!.nextRunAt).toBeNull(); + }); + + it('re-arms a completed once-job when the schedule itself is edited', async () => { + const job = svc.service.createJob( + mkInput({ scheduleType: 'once', runAt: Date.now() - 1000, intervalMinutes: undefined }) + ); + await svc.service.tickDueJobs(Date.now()); + await flush(); + expect(svc.service.getJob(job.id)!.completedOnce).toBe(true); + + const future = Date.now() + 3_600_000; + const updated = svc.service.updateJob(job.id, { runAt: future, enabled: true }); + expect(updated!.completedOnce).toBe(false); + expect(updated!.nextRunAt).toBe(future); + }); + + it('rejects a partial update that leaves an inconsistent schedule (no dead enabled job)', () => { + const job = svc.service.createJob(mkInput({ scheduleType: 'interval', intervalMinutes: 10 })); + // Switch to 'once' WITHOUT a runAt → would otherwise yield a dead nextRunAt:null. + expect(() => svc.service.updateJob(job.id, { scheduleType: 'once', intervalMinutes: undefined })).toThrow(); + // The stored job is left untouched. + const after = svc.service.getJob(job.id)!; + expect(after.scheduleType).toBe('interval'); + expect(after.nextRunAt).not.toBeNull(); + }); }); describe('deleteJob', () => { @@ -240,9 +275,13 @@ describe('CronService', () => { await local.service.tickDueJobs(fireAt); await flush(); - // No run recorded, but the schedule still advanced past the skipped slot. - expect(local.service.listRuns(job.id).length).toBe(0); + // A 'skipped' run is recorded (so the history isn't silently empty), and + // the schedule still advanced past the skipped slot. + const runs = local.service.listRuns(job.id); + expect(runs.length).toBe(1); + expect(runs[0].status).toBe('skipped'); const after = local.service.getJob(job.id)!; + expect(after.lastStatus).toBe('skipped'); expect(after.nextRunAt!).toBeGreaterThan(fireAt); expect(after.lastDueKey).not.toBeNull(); });