diff --git a/cron/jobs.py b/cron/jobs.py index ed0ac61fb21..6ec6d5be123 100644 --- a/cron/jobs.py +++ b/cron/jobs.py @@ -31,7 +31,7 @@ except ImportError: # pragma: no cover - non-Windows msvcrt = None from datetime import datetime, timedelta from pathlib import Path -from hermes_constants import get_hermes_home +from hermes_constants import get_default_hermes_root, get_hermes_home from typing import Optional, Dict, List, Any, Union logger = logging.getLogger(__name__) @@ -49,7 +49,7 @@ except ImportError: # Configuration # ============================================================================= -HERMES_DIR = get_hermes_home().resolve() +HERMES_DIR = get_default_hermes_root().resolve() CRON_DIR = HERMES_DIR / "cron" JOBS_FILE = CRON_DIR / "jobs.json" # Heartbeat file the in-process ticker touches on every loop iteration. The @@ -615,10 +615,44 @@ def get_ticker_success_age() -> Optional[float]: # Job CRUD Operations # ============================================================================= +_WARNED_ORPHAN_STORE = False + + +def _warn_if_orphaned_profile_store() -> None: + """Loudly warn (once) if the root store is empty but a profile-local + jobs.json exists from before #32091's root-anchoring fix. + + Such a file is now unreachable (the store anchors at the default root, not + the active profile). The jobs in it were already orphaned pre-fix (the + profile-less gateway never read them), so this is not a regression — but a + user who could SEE them in `cron list` under their profile would otherwise + find them silently gone. Point them at the path instead of failing silent. + """ + global _WARNED_ORPHAN_STORE + if _WARNED_ORPHAN_STORE: + return + try: + active = get_hermes_home().resolve() + if active == HERMES_DIR: + return # not in a profile; nothing could be orphaned + legacy = active / "cron" / "jobs.json" + if legacy.exists(): + _WARNED_ORPHAN_STORE = True + logger.warning( + "Cron jobs now live at %s (shared across profiles). A legacy " + "profile-local store exists at %s and is no longer read; " + "re-create those jobs or move them into the root store. (#32091)", + JOBS_FILE, legacy, + ) + except Exception: + pass # best-effort advisory; never block load_jobs + + def load_jobs() -> List[Dict[str, Any]]: """Load all jobs from storage.""" ensure_dirs() if not JOBS_FILE.exists(): + _warn_if_orphaned_profile_store() return [] _strict_retry = False # track whether we used the strict=False fallback diff --git a/cron/scheduler.py b/cron/scheduler.py index bd6d2b5359f..b7d662e61a4 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -283,9 +283,17 @@ def _get_hermes_home() -> Path: def _get_lock_paths() -> tuple[Path, Path]: - """Resolve cron lock paths at call time so profile/env changes are honored.""" - hermes_home = _get_hermes_home() - lock_dir = hermes_home / "cron" + """Resolve cron lock paths at call time so profile/env changes are honored. + + Anchored on the DEFAULT ROOT home (not the active profile), matching the + jobs store in cron.jobs (which uses get_default_hermes_root). The tick lock + is storage-coordination — it must live next to the single jobs.json so that + tickers running under different profiles share one lock and can't + double-fire the relocated store (#32091). Execution context (.env, + config.yaml, scripts) stays profile-aware via _get_hermes_home(). + """ + from hermes_constants import get_default_hermes_root + lock_dir = (_hermes_home or get_default_hermes_root()) / "cron" return lock_dir, lock_dir / ".tick.lock" diff --git a/cron/suggestions.py b/cron/suggestions.py index 636a0335cc3..6c10a4f5b28 100644 --- a/cron/suggestions.py +++ b/cron/suggestions.py @@ -36,13 +36,13 @@ import uuid from pathlib import Path from typing import Any, Dict, List, Optional -from hermes_constants import get_hermes_home +from hermes_constants import get_default_hermes_root from hermes_time import now as _hermes_now from utils import atomic_replace logger = logging.getLogger(__name__) -CRON_DIR = get_hermes_home().resolve() / "cron" +CRON_DIR = get_default_hermes_root().resolve() / "cron" SUGGESTIONS_FILE = CRON_DIR / "suggestions.json" # In-process lock protecting load->modify->save cycles (the background review diff --git a/scripts/release.py b/scripts/release.py index e10ffcb7144..9b60b51f939 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -1225,6 +1225,7 @@ AUTHOR_MAP = { "holynn@placeholder.local": "holynn-q", "agent@hermes.local": "jacdevos", "sunsky.lau@gmail.com": "liuhao1024", + "mohamed.origami@gmail.com": "mohamedorigami-jpg", # PR #32117 (cron storage root anchor; #32091) "rob@rbrtbn.com": "rbrtbn", "haaasined@gmail.com": "VinciZhu", "fabianoeq@gmail.com": "rodrigoeqnit", diff --git a/tests/cron/test_claim_job_for_fire.py b/tests/cron/test_claim_job_for_fire.py index abbe969eb04..a02b1110381 100644 --- a/tests/cron/test_claim_job_for_fire.py +++ b/tests/cron/test_claim_job_for_fire.py @@ -14,7 +14,10 @@ import pytest def temp_home(tmp_path, monkeypatch): """Isolated HERMES_HOME so jobs.json doesn't touch the real store.""" monkeypatch.setenv("HERMES_HOME", str(tmp_path)) - # cron.jobs caches no home at import; get_hermes_home() reads the env live. + # NOTE: cron.jobs resolves its store paths (JOBS_FILE, CRON_DIR) from + # get_default_hermes_root() at IMPORT time, so setting HERMES_HOME here does + # not re-point an already-imported module's store. These tests exercise the + # claim logic on in-memory job dicts and don't depend on the on-disk path. yield tmp_path diff --git a/tests/cron/test_cron_profile_storage.py b/tests/cron/test_cron_profile_storage.py new file mode 100644 index 00000000000..e13a1333d2f --- /dev/null +++ b/tests/cron/test_cron_profile_storage.py @@ -0,0 +1,105 @@ +"""Regression tests for #32091 — profile-scoped cron jobs orphaned. + +Cron storage (CRON_DIR/JOBS_FILE) must anchor at the *default root* Hermes +home, not the active profile's home. Otherwise a job created from a +profile-scoped agent session writes to ~/.hermes/profiles/

/cron/jobs.json, +while the profile-less gateway reads only ~/.hermes/cron/jobs.json — the job +is silently orphaned (looks healthy in `list`, never fires). +""" +import importlib +import os +from pathlib import Path + + +def test_cron_storage_anchors_at_root_under_profile(tmp_path, monkeypatch): + """Under a profile HERMES_HOME (/profiles/), the cron store + resolves to /cron, NOT /profiles//cron.""" + root = tmp_path / "hermes_home" + profile_home = root / "profiles" / "myprofile" + profile_home.mkdir(parents=True) + + # Pretend the platform default root IS our tmp root, and the active + # HERMES_HOME is a profile under it (the #32091 scenario). + import hermes_constants + monkeypatch.setattr(hermes_constants, "_get_platform_default_hermes_home", + lambda: root) + monkeypatch.setenv("HERMES_HOME", str(profile_home)) + + # get_default_hermes_root must return the ROOT, not the profile dir. + assert hermes_constants.get_default_hermes_root().resolve() == root.resolve() + # ...while get_hermes_home (used elsewhere) follows the profile override. + assert hermes_constants.get_hermes_home().resolve() == profile_home.resolve() + + # cron/jobs.py computes HERMES_DIR from get_default_hermes_root at import, + # so a fresh import under this env anchors the store at /cron. + import cron.jobs as jobs + importlib.reload(jobs) + try: + assert jobs.HERMES_DIR.resolve() == root.resolve() + assert jobs.JOBS_FILE.resolve() == (root / "cron" / "jobs.json").resolve() + # The orphan path (/cron/jobs.json) must NOT be the store. + assert jobs.JOBS_FILE.resolve() != (profile_home / "cron" / "jobs.json").resolve() + finally: + # Restore module state for other tests (reload under the real env). + monkeypatch.undo() + importlib.reload(jobs) + + +def test_cron_storage_unaffected_when_no_profile(tmp_path, monkeypatch): + """With no profile (HERMES_HOME == root), behavior is unchanged: store at + /cron.""" + root = tmp_path / "hermes_home" + root.mkdir(parents=True) + import hermes_constants + monkeypatch.setattr(hermes_constants, "_get_platform_default_hermes_home", + lambda: root) + monkeypatch.setenv("HERMES_HOME", str(root)) + + import cron.jobs as jobs + importlib.reload(jobs) + try: + assert jobs.JOBS_FILE.resolve() == (root / "cron" / "jobs.json").resolve() + finally: + monkeypatch.undo() + importlib.reload(jobs) + + +def test_tick_lock_anchors_at_root_under_profile(tmp_path, monkeypatch): + """The cron tick lock must live at /cron/.tick.lock, NOT the profile + dir — otherwise tickers under different profiles grab different locks and + double-fire the (now root-anchored) jobs store (#32091).""" + import importlib + root = tmp_path / "hermes_home" + profile_home = root / "profiles" / "p" + profile_home.mkdir(parents=True) + import hermes_constants + monkeypatch.setattr(hermes_constants, "_get_platform_default_hermes_home", lambda: root) + monkeypatch.setenv("HERMES_HOME", str(profile_home)) + import cron.scheduler as sched + importlib.reload(sched) + try: + # _hermes_home override is None -> uses get_default_hermes_root() + sched._hermes_home = None + lock_dir, lock_file = sched._get_lock_paths() + assert lock_dir.resolve() == (root / "cron").resolve() + assert lock_file.resolve() == (root / "cron" / ".tick.lock").resolve() + assert lock_dir.resolve() != (profile_home / "cron").resolve() + finally: + monkeypatch.undo() + importlib.reload(sched) + + +def test_get_default_hermes_root_docker_layouts(tmp_path, monkeypatch): + """get_default_hermes_root resolves the root for Docker/custom HERMES_HOME + (outside ~/.hermes), so cron storage works in containers.""" + import hermes_constants + native = tmp_path / "native_home" + monkeypatch.setattr(hermes_constants, "_get_platform_default_hermes_home", lambda: native) + + # Docker custom root (outside native): HERMES_HOME itself IS the root. + monkeypatch.setenv("HERMES_HOME", "/opt/data") + assert hermes_constants.get_default_hermes_root() == Path("/opt/data") + + # Docker profile layout: /profiles/ -> . + monkeypatch.setenv("HERMES_HOME", "/opt/data/profiles/coder") + assert hermes_constants.get_default_hermes_root() == Path("/opt/data")