mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-25 11:02:03 +00:00
fix(cron): anchor cron storage at the default root home (not the active profile)
`cron/jobs.py` resolved `HERMES_DIR`/`JOBS_FILE` from `get_hermes_home()`, which follows the active profile override. So a job created from a profile-scoped agent session (`hermes -p myprofile chat`, where the in-process `cronjob` tool calls `create_job`) was written to `~/.hermes/profiles/myprofile/cron/jobs.json`, while the profile-less gateway (`hermes gateway run`) reads only `~/.hermes/cron/jobs.json`. The job was silently orphaned: `cronjob action=list` from the same profile reported it healthy (same file), but the gateway ticker never saw it and it never fired. `last_run_at` stayed null forever. (#32091) Fix: resolve the cron store from `get_default_hermes_root()` — the purpose-built "profile-level operations" root that returns `<root>` even when `HERMES_HOME` is `<root>/profiles/<name>` (and handles Docker/custom layouts). Now the creator, the gateway scheduler, and the dashboard all agree on a single jobs.json at the root, so a job created under any profile is visible to the gateway. Scope: this is the storage-location half of the fix. Making a job *execute* under its originating profile's config/skills (a per-job `profile` field + runtime context scoping, the #48649 sibling) is a separate, riskier change and will follow as its own PR — keeping this layer minimal and safe. Salvaged from #32117 by @mohamedorigami-jpg (authorship preserved). The comprehensive #33839 (@sweetcornna) takes the same Option-A storage approach and additionally adds the per-job profile execution scoping; this PR lands the safe storage layer first. Tests: `tests/cron/test_cron_profile_storage.py` — asserts the store anchors at `<root>/cron` under a profile HERMES_HOME (not `<profile>/cron`), and is unchanged when no profile is active. Full `tests/cron/` suite: 511 passed. Fixes #32091 Co-authored-by: mohamedorigami-jpg <mohamed.origami@gmail.com>
This commit is contained in:
parent
7b9a0b315b
commit
a5c09fd176
5 changed files with 158 additions and 8 deletions
38
cron/jobs.py
38
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
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
||||
|
|
|
|||
105
tests/cron/test_cron_profile_storage.py
Normal file
105
tests/cron/test_cron_profile_storage.py
Normal file
|
|
@ -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/<p>/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 (<root>/profiles/<name>), the cron store
|
||||
resolves to <root>/cron, NOT <root>/profiles/<name>/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 <root>/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 (<profile>/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
|
||||
<root>/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 <root>/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: <custom>/profiles/<name> -> <custom>.
|
||||
monkeypatch.setenv("HERMES_HOME", "/opt/data/profiles/coder")
|
||||
assert hermes_constants.get_default_hermes_root() == Path("/opt/data")
|
||||
Loading…
Add table
Add a link
Reference in a new issue