mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-05 23:19:43 +02:00
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PmvZR12aX2v8K7YhqxPUAU
This commit is contained in:
@@ -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;
|
||||
|
||||
+1
-1
@@ -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';
|
||||
|
||||
Reference in New Issue
Block a user