fix: normalize cron repeat updates - #18416
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the scalar-repeat mismatch; the premise is still valid on current main: gateway/platforms/api_server.py:3672 forwards PATCH values to cron/jobs.py:update_job, which directly merges updates at cron/jobs.py:1220.
Problems
- Current one-shot execution calls
claim_dispatch()beforemark_job_run()(cron/scheduler.py:3372).claim_dispatch()reads raw persistedrepeatand calls.get()atcron/jobs.py:1494-1497, so a legacy scalar one-shot can still fail before the proposed mark-time repair runs._get_due_jobs_locked()has the same raw-map assumption atcron/jobs.py:1814-1817. - The diff also adds per-job profile storage (
cron/jobs.py:671-692,:798-853) without a scheduler consumer or API exposure. That is unrelated to the stated fix and leaves dead persisted state.
Suggested changes
- Salvage repeat normalization onto current
update_job, then cover all raw scheduler consumers while preserving the current pre-claim one-shot invariant. - Add an API-to-storage-to-one-shot-claim regression, and drop the unrelated profile delta.
This is an automated hermes-sweeper review.
| @@ -848,6 +942,18 @@ def update_job(job_id: str, updates: Dict[str, Any]) -> Optional[Dict[str, Any]] | |||
| else: | |||
| updates["workdir"] = _normalize_workdir(_wd) | |||
|
|
|||
| if "repeat" in updates: | |||
| updates["repeat"] = _normalize_repeat_state(updates["repeat"], job.get("repeat")) | |||
There was a problem hiding this comment.
This protects new updates, but current main's claim_dispatch() reads the raw stored value and calls .get() before mark_job_run() executes (cron/jobs.py:1494-1497). Extend legacy repair to that pre-run path and add a scalar one-shot regression.
| @@ -703,6 +795,7 @@ def create_job( | |||
| normalized_toolsets = [str(t).strip() for t in enabled_toolsets if str(t).strip()] if enabled_toolsets else None | |||
| normalized_toolsets = normalized_toolsets or None | |||
| normalized_workdir = _normalize_workdir(workdir) | |||
| normalized_profile = _normalize_profile(profile) | |||
There was a problem hiding this comment.
This profile plumbing is outside the repeat fix. The PR-head scheduler has no job-profile consumer and the API allowlist does not expose profile; please drop it from this PR or land it separately with an execution-path implementation and tests.
c98b940 to
999668d
Compare
Fixes #15582
Related:
nullclearing, completed-count preservation, and legacy persisted-state repair.Cron repeat updates now stay in the canonical scheduler shape even when callers use Web UI/API-style scalar or
nullpayloads.Changes
{ times, completed }instead of persisting raw scalar values.repeat: nullon update to clear a repeat limit.0, negative numbers, strings, floats, booleans).get_job,list_jobs, andmark_job_run, so already-corrupted persisted jobs do not wait until the next run to fail.Why
The Web UI edit surface sends
repeatas a scalar integer ornull, while cron jobs are stored with repeat metadata as an object. Without backend normalization, a PATCH such as{ "repeat": 3 }can persistrepeat: 3; the next run then crashes when repeat accounting expects.get()/completed.Earlier fixes covered the core scalar-to-dict path, but not all edge cases seen from the Web UI/API boundary. This PR is intended to supersede those variants with the full persisted-state contract covered in tests.
Verification
Passed:
Results:
144 passed, 42 warningsfor targeted cron/API/tool tests431 passedfor broader cron + web-server coverageKnown:
gh pr checkscurrently reports no checks for this branch, so local verification above is the available gate.