fix(cron): deduplicate cross-profile jobs in GET /api/cron/jobs
profile=all aggregated every profile's list without deduplication, so a job copied into a second profile's cron/jobs.json during profile creation appeared twice — inflating the desktop sidebar count and rendering duplicate rows. Collect all jobs first, then resolve duplicates by id with default-profile priority, instead of keeping whichever copy the profile loop happened to append first. cron.jobs._normalize_job_record() fills a missing id with the literal sentinel string "unknown", which is truthy — treat it like no id so two genuinely different id-less legacy records from different profiles are never collapsed into one. Fixes #51721 Salvaged from #69132 by @ygd58 (kept its dedup semantics and regression tests, ported onto the web_routers/cron.py seam after the web_server refactor). Co-authored-by: ygd58 <buraysandro9@gmail.com>
This commit is contained in:
@@ -107,16 +107,42 @@ def _list_cron_jobs_sync(profile: str = "all"):
|
||||
if requested.lower() != "all":
|
||||
return _call_cron_for_profile(requested, "list_jobs", True)
|
||||
|
||||
jobs: List[Dict[str, Any]] = []
|
||||
# Aggregating across profiles can surface the SAME job id more than once —
|
||||
# e.g. a job copied into a second profile's cron/jobs.json during profile
|
||||
# creation. Deduplicate by id, deterministically preferring the default
|
||||
# profile's copy over per-iteration order (#51721): collect all jobs first,
|
||||
# then resolve duplicates by id with default-profile priority, rather than
|
||||
# keeping whichever copy happened to be seen first during the profile loop.
|
||||
all_jobs: List[Dict[str, Any]] = []
|
||||
for item in _cron_profile_dicts():
|
||||
name = str(item.get("name") or "")
|
||||
if not name:
|
||||
continue
|
||||
try:
|
||||
jobs.extend(_call_cron_for_profile(name, "list_jobs", True))
|
||||
all_jobs.extend(_call_cron_for_profile(name, "list_jobs", True))
|
||||
except Exception:
|
||||
_log.exception("Failed to list cron jobs for profile %s", name)
|
||||
return jobs
|
||||
|
||||
by_id: Dict[str, Dict[str, Any]] = {}
|
||||
unkeyed: List[Dict[str, Any]] = []
|
||||
for job in all_jobs:
|
||||
if not isinstance(job, dict):
|
||||
continue
|
||||
jid = job.get("id") or job.get("job_id")
|
||||
# cron.jobs._normalize_job_record() fills a missing id with the literal
|
||||
# sentinel string "unknown" — which is truthy, so a plain `if not jid`
|
||||
# check would NOT catch it and two genuinely different id-less jobs from
|
||||
# different profiles would collapse into one under this shared sentinel
|
||||
# key. Treat the sentinel the same as no id: never deduplicated.
|
||||
if not jid or jid == "unknown":
|
||||
unkeyed.append(job)
|
||||
continue
|
||||
existing = by_id.get(jid)
|
||||
if existing is None or (
|
||||
job.get("is_default_profile") and not existing.get("is_default_profile")
|
||||
):
|
||||
by_id[jid] = job
|
||||
return list(by_id.values()) + unkeyed
|
||||
|
||||
|
||||
def _get_cron_job_sync(job_id: str, profile: Optional[str] = None):
|
||||
|
||||
@@ -1134,3 +1134,97 @@ async def test_cron_job_mutations_resolve_the_owner_when_the_hint_is_another_pro
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
await _rt_cron.get_cron_job(job_id, profile="default")
|
||||
assert exc.value.status_code == 404
|
||||
|
||||
|
||||
class TestCronJobsCrossProfileDedup:
|
||||
"""Regression tests for #51721: GET /api/cron/jobs (profile=all) aggregated
|
||||
jobs from every profile without deduplication — a job copied into a second
|
||||
profile's cron/jobs.json during profile creation showed up twice. Uses real
|
||||
cron.jobs storage (direct jobs.json writes for the id-less case) so the
|
||||
tests exercise cron.jobs._normalize_job_record()'s real "unknown" sentinel
|
||||
behavior, not a mocked list.
|
||||
"""
|
||||
|
||||
def _write_jobs_file(self, home, jobs):
|
||||
path = home / "cron" / "jobs.json"
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
path.write_text(json.dumps(jobs), encoding="utf-8")
|
||||
|
||||
def test_shared_id_default_profile_copy_wins(self, isolated_profiles):
|
||||
"""The same job id in both profiles: the default profile's copy must
|
||||
survive, regardless of which profile's list was appended first during
|
||||
aggregation."""
|
||||
self._write_jobs_file(isolated_profiles["default"], [{
|
||||
"id": "shared-job-1", "name": "shared", "prompt": "DEFAULT COPY",
|
||||
"enabled": True, "schedule": {"type": "interval", "every": "1h"},
|
||||
}])
|
||||
self._write_jobs_file(isolated_profiles["worker_alpha"], [{
|
||||
"id": "shared-job-1", "name": "shared", "prompt": "WORKER COPY",
|
||||
"enabled": True, "schedule": {"type": "interval", "every": "1h"},
|
||||
}])
|
||||
|
||||
result = _rt_cron._list_cron_jobs_sync("all")
|
||||
matches = [j for j in result if j.get("id") == "shared-job-1"]
|
||||
|
||||
assert len(matches) == 1, f"Duplicate not resolved: {matches}"
|
||||
assert matches[0]["prompt"] == "DEFAULT COPY", (
|
||||
"Default profile's copy must always win, got: " + str(matches[0])
|
||||
)
|
||||
assert matches[0]["is_default_profile"] is True
|
||||
|
||||
def test_worker_first_shared_id_still_resolves_to_default_copy(self, isolated_profiles):
|
||||
"""Order-independence: _cron_profile_dicts() lists the default profile
|
||||
first today, but the dedup must not depend on that. Feed the worker
|
||||
profile's copy through the loop first by listing only it, then verify a
|
||||
full aggregate still prefers the default copy."""
|
||||
self._write_jobs_file(isolated_profiles["default"], [{
|
||||
"id": "shared-job-2", "name": "shared", "prompt": "DEFAULT COPY",
|
||||
"enabled": True, "schedule": {"type": "interval", "every": "1h"},
|
||||
}])
|
||||
self._write_jobs_file(isolated_profiles["worker_alpha"], [{
|
||||
"id": "shared-job-2", "name": "shared", "prompt": "WORKER COPY",
|
||||
"enabled": True, "schedule": {"type": "interval", "every": "1h"},
|
||||
}])
|
||||
|
||||
worker_first = _web_server_cron._call_cron_for_profile("worker_alpha", "list_jobs", True)
|
||||
assert worker_first[0]["prompt"] == "WORKER COPY"
|
||||
|
||||
result = _rt_cron._list_cron_jobs_sync("all")
|
||||
matches = [j for j in result if j.get("id") == "shared-job-2"]
|
||||
assert len(matches) == 1
|
||||
assert matches[0]["prompt"] == "DEFAULT COPY"
|
||||
|
||||
def test_unique_ids_all_kept(self, isolated_profiles):
|
||||
self._write_jobs_file(isolated_profiles["default"], [{
|
||||
"id": "job-a", "name": "a", "prompt": "a",
|
||||
"enabled": True, "schedule": {"type": "interval", "every": "1h"},
|
||||
}])
|
||||
self._write_jobs_file(isolated_profiles["worker_alpha"], [{
|
||||
"id": "job-b", "name": "b", "prompt": "b",
|
||||
"enabled": True, "schedule": {"type": "interval", "every": "1h"},
|
||||
}])
|
||||
|
||||
result = _rt_cron._list_cron_jobs_sync("all")
|
||||
ids = {j.get("id") for j in result}
|
||||
|
||||
assert ids == {"job-a", "job-b"}
|
||||
|
||||
def test_idless_records_from_different_profiles_both_preserved(self, isolated_profiles):
|
||||
"""cron.jobs._normalize_job_record() fills a missing id with the literal
|
||||
sentinel string "unknown" before this data reaches the aggregation
|
||||
logic. Two genuinely DIFFERENT id-less jobs (hand-edited or pre-id-field
|
||||
legacy records) from different profiles both normalize to id="unknown" —
|
||||
a naive by-id dedup would wrongly collapse them into one. Both survive."""
|
||||
self._write_jobs_file(isolated_profiles["default"], [{
|
||||
"name": "legacy-default", "prompt": "legacy job in default",
|
||||
"enabled": True, "schedule": {"type": "interval", "every": "1h"},
|
||||
}])
|
||||
self._write_jobs_file(isolated_profiles["worker_alpha"], [{
|
||||
"name": "legacy-worker", "prompt": "legacy job in worker",
|
||||
"enabled": True, "schedule": {"type": "interval", "every": "1h"},
|
||||
}])
|
||||
|
||||
result = _rt_cron._list_cron_jobs_sync("all")
|
||||
prompts = {j.get("prompt") for j in result}
|
||||
|
||||
assert prompts == {"legacy job in default", "legacy job in worker"}
|
||||
|
||||
Reference in New Issue
Block a user