← Back to PRs

#19372: fix(cron): normalize jobId → id for file-backed jobs

by namabile open 2026-02-17 18:40 View on GitHub →
size: M
Fixes #19300 ## Summary File-backed cron jobs that use `jobId` instead of `id` in `jobs.json` (a common convention in hand-written configs) were invisible to `cron run`, `cron update`, `cron remove`, and `cron enable/disable` because `findJobOrThrow()` searches by `j.id`. The scheduler was unaffected because it iterates the array without looking up by id, and `cron list` returned jobs normally since it returns the full array — making this particularly confusing to diagnose. ## Root cause `ensureLoaded()` in `src/cron/service/store.ts` normalizes ~10 fields during the store migration loop (name, description, sessionKey, enabled, payload, schedule, delivery, etc.) but never normalized `jobId` → `id`. When a job had `jobId` but no `id`, `j.id` was `undefined` at runtime, causing `findJobOrThrow()` to fail with `"unknown cron job id"`. ## Fix Added `jobId` → `id` normalization in `ensureLoaded()` alongside the existing field migrations: - If a job has `jobId` but no `id`, the value is copied to `id` and `jobId` is removed - If neither field is present, a UUID is generated - When both `id` and `jobId` are present, `id` takes precedence - The normalized id is persisted back to the store file ## Testing 7 regression tests covering: - `cron.run` succeeds with normalized `jobId` - `cron.list` returns proper `id` field - `getJob` finds file-backed jobs by normalized id - UUID generation when neither `id` nor `jobId` is present - Existing `id` field is preserved - `id` takes precedence over `jobId` when both present - Normalized id is persisted to disk All 151 existing cron tests continue to pass with zero regressions. AI-assisted: This fix was developed with Claude Code. Testing was done against the full cron test suite. <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR fixes a real bug (#19300) where file-backed cron jobs using `jobId` instead of `id` were invisible to `cron run`, `cron update`, and `cron remove`. The normalization added to `ensureLoaded()` in `store.ts` correctly handles the main case, and the 7 regression tests provide solid coverage. One incomplete case in the fix: - When a job has **both** a valid `id` and a legacy `jobId`, the guard `if (typeof raw.id !== "string" || !raw.id.trim())` short-circuits the whole block, so `jobId` is never deleted. It gets passed through the `as CronJob[]` cast and written back to disk. The primary bug (lookup failure) is resolved since `findJobOrThrow` uses `j.id`, but the stray `jobId` field lingers in the persisted store — inconsistent with the fix's stated goal of stripping it. The corresponding test also lacks an assertion verifying that `jobId` was removed in this scenario. <h3>Confidence Score: 3/5</h3> - Safe to merge for the primary fix; the stray `jobId` in the both-fields case is a minor correctness gap that does not reintroduce the original bug. - The fix correctly resolves the reported bug and is well-tested for the common cases. Score is 3 rather than 4-5 because there is an incomplete cleanup path when both `id` and `jobId` are present — `jobId` is not deleted from the persisted store in that scenario, and the test for it does not assert this. The primary lookup issue is fully resolved. - Pay close attention to `src/cron/service/store.ts` lines 254–263 — the `else if ("jobId" in raw)` cleanup branch is missing. <sub>Last reviewed commit: 50dd40a</sub> <!-- greptile_other_comments_section --> <sub>(3/5) Reply to the agent's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!</sub> <!-- /greptile_comment -->

Most Similar PRs