disk-cleanup incorrectly deleting cron/jobs.json registry
NousResearch/hermes-agent · cluster 32164 · cron
Data as of
Nothing open on GitHub. Fix merged in #34840. Check it covers your case.
The short answer
- Known?
- Yes. 13 related items grouped here, 2 issues already closed.
- Fix event
- A fix was merged to main in PR #34840. Check it covers your case. No release data is tracked.
- Tracker
- The maintainers' tracker marks this
resolved, deciding PRs #34840, #39067. That is separate from the fix event. - Still open
- Nothing open on GitHub.
Multiple issues and proposed fixes addressing a bug where disk-cleanup misclassifies cron/jobs.json as cron-output and may delete it. The fixes focus on restricting cron-output classification to the correct subtree and revalidating stale entries before deletion.
Open issues0
Open PRs0
Fixes on record
-
#34840 fix(cron): stop silent cron-job loss across update (disk-cleanup + snapshot restore)
files:
hermes_cli/backup.py,hermes_cli/main.py,plugins/disk-cleanup/disk_cleanup.py,scripts/release.py,tests/hermes_cli/test_backup.py,tests/plugins/test_disk_cleanup_plugin.pyShow the diff
-
hermes_cli/backup.py@@ -670,6 +670,105 @@ def restore_quick_snapshot( return restored > 0 +# Relative path of the cron job database inside HERMES_HOME. Kept in sync with +# the entry in ``_QUICK_STATE_FILES`` and with ``cron/jobs.py``'s ``JOBS_FILE``. +_CRON_JOBS_REL = "cron/jobs.json" + + +def _count_cron_jobs(path: Path) -> Optional[int]: + """Return the number of cron jobs stored in ``path``. + + The canonical on-disk shape is ``{"jobs": [...]}`` (see ``cron/jobs.py``). + A legacy bare-list shape (``[...]``) is also honoured. + + Returns: + The job count for any *valid, readable* JSON document, or ``None`` if + the file is missing or cannot be parsed. ``None`` means "unknown" — + callers must not treat it as "zero jobs", because acting on an + unreadable file could mask a real corruption the user needs to see. + """ + if not path.is_file(): + return None + try: + with open(path, "r", encoding="utf-8") as f: + data = json.load(f) + except (OSError, json.JSONDecodeError): + return None + if isinstance(data, dict): + jobs = data.get("jobs", []) + return len(jobs) if isinstance(jobs, list) else None + if isinstance(data, list): + return len(data) + return None + + +def restore_cron_jobs_if_emptied( + snapshot_id: str, + hermes_home: Optional[Path] = None, +) -> Optional[Dict[str, Any]]: + """Safety net for silent cron-job loss across ``hermes update``. + + Config-version migrations have been observed to leave ``cron/jobs.json`` + valid-but-empty after an update, silently dropping every scheduled job + (issue #34600). The existing malformed-shape guards in ``cron/jobs.py`` + don't catch this case because ``{"jobs": []}`` is perfectly valid JSON. + + This compares the *current* job count against the pre-update snapshot. If + the live file now has **zero** jobs while the snapshot captured **one or + more**, the snapshot copy of ``cron/jobs.json`` is restored in place. + + The check is deliberately conservative — it only ever restores when there + is unambiguous evidence of loss (snapshot had jobs, live file has none), + so a user who genuinely deleted all their jobs during/after the update is + never second-guessed, and an unreadable live file (count ``None``) is left + untouched so real corruption still surfaces. + + Args: + snapshot_id: The pre-update quick-snapshot id (from + :func:`create_quick_snapshot`). + hermes_home: Override for the Hermes home directory (tests). + + Returns: + ``None`` when no action was taken (the common, healthy path). On a + successful restore, a dict ``{"restored": True, "job_count": N, + "snapshot_id": ...}`` so the caller can warn the user. + """ + if not snapshot_id: + return None + + home = hermes_home or get_hermes_home() + live_path = home / _CRON_JOBS_REL + + live_count = _count_cron_jobs(live_path) + # Only act when the live file is readable AND empty. ``None`` (missing or + # unparseable) is intentionally left alone — that's a different failure + # mode the user should see rather than have papered over. + if live_count is None or live_count > 0: + return None + + snap_path = _quick_snapshot_root(home) / snapshot_id / _CRON_JOBS_REL + snap_count = _count_cron_jobs(snap_path) + if not snap_count: # None or 0 — nothing worth restoring + return None + + try: + live_path.parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(snap_path, live_path) + except (OSError, PermissionError) as exc: + logger.error( + "Cron jobs were emptied during update but auto-restore failed: %s", exc + ) + return None + + logger.warning( + "Restored %d cron job(s) from pre-update snapshot %s " + "(cron/jobs.json was emptied during migration)", + snap_count, + snapshot_id, + ) + return {"restored": True, "job_count": snap_count, "snapshot_id": snapshot_id} + + def _prune_quick_snapshots(root: Path, keep: int = _QUICK_DEFAULT_KEEP) -> int: """Remove oldest quick snapshots beyond the keep limit. Returns count deleted.""" if not root.exists(): -
hermes_cli/main.py@@ -9125,12 +9125,13 @@ def _cmd_update_impl(args, gateway_mode: bool): # though `git pull` can't touch $HERMES_HOME, this is cheap # belt-and-suspenders insurance and gives the user something to # restore from via `/snapshot list` / `/snapshot restore <id>`. + pre_update_snapshot_id = None try: from hermes_cli.backup import create_quick_snapshot - snap_id = create_quick_snapshot(label="pre-update", keep=1) - if snap_id: - print(f" ✓ Pre-update snapshot: {snap_id}") + pre_update_snapshot_id = create_quick_snapshot(label="pre-update", keep=1) + if pre_update_snapshot_id: + print(f" ✓ Pre-update snapshot: {pre_update_snapshot_id}") except Exception as exc: # Never let a snapshot failure block an update. logger.debug("Pre-update snapshot failed: %s", exc) @@ -9467,6 +9468,25 @@ def _cmd_update_impl(args, gateway_mode: bool): else: print(" ✓ Configuration is up to date") + # Safety net: config-version migrations have been observed to leave + # cron/jobs.json valid-but-empty, silently dropping every scheduled + # job (issue #34600). If the live file is now empty while the + # pre-update snapshot held jobs, restore it and warn loudly. + try: + from hermes_cli.backup import restore_cron_jobs_if_emptied + + cron_restore = restore_cron_jobs_if_emptied(pre_update_snapshot_id) + if cron_restore: + print() + print( + " ⚠️ cron/jobs.json was emptied during this update — " + f"restored {cron_restore['job_count']} job(s) from " + f"pre-update snapshot {cron_restore['snapshot_id']}." + ) + except Exception as exc: + # Never let the cron safety net break an otherwise-good update. + logger.debug("Cron jobs auto-restore check failed: %s", exc) + print() print("✓ Update complete!") -
plugins/disk-cleanup/disk_cleanup.py@@ -481,7 +481,14 @@ def guess_category(path: Path) -> Optional[str]: }: return None if top == "cron" or top == "cronjobs": - return "cron-output" + # Only files under the disposable ``output/`` subtree are + # cleanup candidates. Top-level cron control-plane state + # (e.g. ``jobs.json``, ``.tick.lock``) must never be + # auto-tracked — deleting it wipes the live scheduler + # registry. See issue #32164. + if len(rel.parts) >= 2 and rel.parts[1] == "output": + return "cron-output" + return None if top == "cache": return "temp" except ValueError: -
scripts/release.py@@ -645,6 +645,7 @@ "[redacted:email]": "AntAISecurityLab", "[redacted:email]": "briandevans", "[redacted:email]": "Bartok9", + "[redacted:email]": "sweetcornna", "[redacted:email]": "CruxExperts", "[redacted:email]": "LeonSGP43", "[redacted:email]": "Kailigithub", -
tests/hermes_cli/test_backup.py@@ -1679,3 +1679,105 @@ def test_does_not_touch_pre_update_backups(self, hermes_home): _t.sleep(1.05) # Update backup must still be there assert update_backup.exists(), "pre-migration rotation wrongly pruned the pre-update backup" + + +# --------------------------------------------------------------------------- +# Cron jobs auto-restore after silent migration loss (issue #34600) +# --------------------------------------------------------------------------- + +class TestRestoreCronJobsIfEmptied: + """`hermes update` config migration can leave cron/jobs.json valid-but-empty, + silently dropping every scheduled job. `restore_cron_jobs_if_emptied` is the + post-migration safety net that restores from the pre-update snapshot.""" + + @staticmethod + def _seed_jobs(path: Path, jobs): + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps({"jobs": jobs})) + + def _make_snapshot(self, hermes_home: Path, label="pre-update"): + from hermes_cli.backup import create_quick_snapshot + return create_quick_snapshot(label=label, hermes_home=hermes_home, keep=5) + + def test_restores_when_emptied_after_migration(self, tmp_path): + from hermes_cli.backup import restore_cron_jobs_if_emptied + hermes_home = tmp_path / ".hermes" + jobs_path = hermes_home / "cron" / "jobs.json" + # Pre-update: 3 real jobs. + self._seed_jobs(jobs_path, [{"id": "a"}, {"id": "b"}, {"id": "c"}]) + snap_id = self._make_snapshot(hermes_home) + assert snap_id + + # Migration silently empties the file (valid JSON, zero jobs). + jobs_path.write_text(json.dumps({"jobs": []})) + + result = restore_cron_jobs_if_emptied(snap_id, hermes_home=hermes_home) + assert result is not None + assert result["restored"] is True + assert result["job_count"] == 3 + assert result["snapshot_id"] == snap_id + + # The live file now has the jobs back. + restored = json.loads(jobs_path.read_text()) + assert len(restored["jobs"]) == 3 + + def test_noop_when_live_file_still_has_jobs(self, tmp_path): + from hermes_cli.backup import restore_cron_jobs_if_emptied + hermes_home = tmp_path / ".hermes" + jobs_path = hermes_home / "cron" / "jobs.json" + self._seed_jobs(jobs_path, [{"id": "a"}, {"id": "b"}]) + snap_id = self._make_snapshot(hermes_home) + + # Healthy path: file unchanged after update. + result = restore_cron_jobs_if_emptied(snap_id, hermes_home=hermes_home) + assert result is None + + def test_noop_when_snapshot_had_no_jobs(self, tmp_path): + from hermes_cli.backup import restore_cron_jobs_if_emptied + hermes_home = tmp_path / ".hermes" + jobs_path = hermes_home / "cron" / "jobs.json" + # Pre-update genuinely had zero jobs; current is also empty. + self._seed_jobs(jobs_path, []) + snap_id = self._make_snapshot(hermes_home) + jobs_path.write_text(json.dumps({"jobs": []})) + + result = restore_cron_jobs_if_emptied(snap_id, hermes_home=hermes_home) + assert result is None + + def test_noop_when_live_file_unreadable(self, tmp_path): + """An unparseable live file is left alone — that's a different failure + mode the user should see, not silently overwrite.""" + from hermes_cli.backup import restore_cron_jobs_if_emptied + hermes_home = tmp_path / ".hermes" + jobs_path = hermes_home / "cron" / "jobs.json" + self._seed_jobs(jobs_path, [{"id": "a"}]) + snap_id = self._make_snapshot(hermes_home) + jobs_path.write_text("{ this is not valid json") + + result = restore_cron_jobs_if_emptied(snap_id, hermes_home=hermes_home) + assert result is None + # File left untouched. + assert jobs_path.read_text() == "{ this is not valid json" + + def test_noop_when_snapshot_id_missing(self, tmp_path): + from hermes_cli.backup import restore_cron_jobs_if_emptied + hermes_home = tmp_path / ".hermes" + jobs_path = hermes_home / "cron" / "jobs.json" + self._seed_jobs(jobs_path, []) + assert restore_cron_jobs_if_emptied(None, hermes_home=hermes_home) is None + assert restore_cron_jobs_if_emptied("", hermes_home=hermes_home) is None + + def test_restores_legacy_bare_list_snapshot_shape(self, tmp_path): + """A legacy snapshot storing a bare JSON list (not {"jobs": [...]}) is + still counted and restored.""" + from hermes_cli.backup import restore_cron_jobs_if_emptied + hermes_home = tmp_path / ".hermes" + jobs_path = hermes_home / "cron" / "jobs.json" + jobs_path.parent.mkdir(parents=True, exist_ok=True) + jobs_path.write_text(json.dumps([{"id": "a"}, {"id": "b"}])) + snap_id = self._make_snapshot(hermes_home) + + jobs_path.write_text(json.dumps({"jobs": []})) + result = restore_cron_jobs_if_emptied(snap_id, hermes_home=hermes_home) + assert result is not None + assert result["job_count"] == 2 -
tests/plugins/test_disk_cleanup_plugin.py@@ -129,12 +129,40 @@ def test_skips_protected_top_level(self, _isolate_env): def test_cron_subtree_categorised(self, _isolate_env): dg = _load_lib() - cron_dir = _isolate_env / "cron" - cron_dir.mkdir() - p = cron_dir / "job_output.md" + # Only files under ``cron/output/`` are disposable run artifacts. + output_dir = _isolate_env / "cron" / "output" / "job_123" + output_dir.mkdir(parents=True) + p = output_dir / "run.md" p.write_text("x") assert dg.guess_category(p) == "cron-output" + def test_cron_jobs_json_not_tracked(self, _isolate_env): + """Regression for #32164: the cron registry must never be tracked.""" + dg = _load_lib() + cron_dir = _isolate_env / "cron" + cron_dir.mkdir() + p = cron_dir / "jobs.json" + p.write_text("[]") + assert dg.guess_category(p) is None + + def test_cron_tick_lock_not_tracked(self, _isolate_env): + """Regression for #32164: cron tick-lock is control-plane state.""" + dg = _load_lib() + cron_dir = _isolate_env / "cron" + cron_dir.mkdir() + p = cron_dir / ".tick.lock" + p.write_text("") + assert dg.guess_category(p) is None + + def test_cronjobs_top_level_not_tracked(self, _isolate_env): + """The legacy ``cronjobs`` alias is also control-plane at the top.""" + dg = _load_lib() + cron_dir = _isolate_env / "cronjobs" + cron_dir.mkdir() + p = cron_dir / "jobs.json" + p.write_text("[]") + assert dg.guess_category(p) is None + def test_ordinary_file_returns_none(self, _isolate_env): dg = _load_lib() p = _isolate_env / "notes.md"
-
-
#39067 fix(cron): re-validate stale cron-output entries before deletion
files:
plugins/disk-cleanup/disk_cleanup.py,tests/plugins/test_disk_cleanup_plugin.pyShow the diff
-
plugins/disk-cleanup/disk_cleanup.py@@ -145,6 +145,33 @@ def save_tracked(tracked: List[Dict[str, Any]]) -> None: } +# Paths under $HERMES_HOME that must NEVER be deleted by quick(), +# regardless of what the stored category says. This is a defense-in-depth +# guard against stale tracked.json entries from before #34840. +_PROTECTED_CRON_PATHS: set[str] = set() + + +def _is_protected_cron_path(p: Path) -> bool: + """Return True if *p* is a cron control-plane file/directory that must + never be deleted. + + This only matches the directory itself and known control-plane files + (``jobs.json``, ``.tick.lock``) — it does NOT blanket-protect + everything under ``cron/`` because ``cron/output/`` is disposable. + """ + # Lazily build the set once per process so HERMES_HOME is resolved + # exactly once. + if not _PROTECTED_CRON_PATHS: + hermes_home = get_hermes_home() + for parent in ("cron", "cronjobs"): + base = hermes_home / parent + _PROTECTED_CRON_PATHS.add(str(base)) + _PROTECTED_CRON_PATHS.add(str(base / "jobs.json")) + _PROTECTED_CRON_PATHS.add(str(base / ".tick.lock")) + resolved = str(p.resolve()) + return resolved in _PROTECTED_CRON_PATHS + + def fmt_size(n: float) -> str: for unit in ("B", "KB", "MB", "GB", "TB"): if n < 1024: @@ -226,6 +253,14 @@ def dry_run() -> Tuple[List[Dict], List[Dict]]: cat = item["category"] size = item["size"] + # Re-validate stale "cron-output" entries (fixes #37721). + if cat == "cron-output": + re_cat = guess_category(p) + if re_cat != "cron-output": + # Stale entry — would be skipped by quick(); omit from + # dry-run output too. + continue + if cat == "test": auto.append(item) elif cat == "temp" and age > 7: @@ -269,6 +304,28 @@ def quick() -> Dict[str, Any]: age = (now - datetime.fromisoformat(item["timestamp"])).days + # ---- stale-state migration (fixes #37721) ---- + # Old tracked.json entries may carry a "cron-output" category for + # paths that are NOT under cron/output/ (e.g. cron/jobs.json). + # guess_category() was fixed in #34840, but existing entries are + # never re-validated. Re-classify here so stale entries for cron + # control-plane state are not deleted. + if cat == "cron-output": + re_cat = guess_category(p) + if re_cat != "cron-output": + _log( + f"SKIP stale cron-output entry: {p} " + f"(re-classified as {re_cat!r})" + ) + # Drop the stale entry — it was misclassified. + continue + + # Hard safety net: never delete cron control-plane state even if + # the category somehow slipped through re-validation above. + if _is_protected_cron_path(p): + _log(f"SKIP protected cron path: {p}") + continue + should_delete = ( cat == "test" or (cat == "temp" and age > 7) -
tests/plugins/test_disk_cleanup_plugin.py@@ -170,6 +170,135 @@ def test_ordinary_file_returns_none(self, _isolate_env): assert dg.guess_category(p) is None +class TestStaleCronEntryMigration: + """Regression tests for #37721 — stale cron-output entries in tracked.json.""" + + def test_quick_skips_stale_cron_output_for_jobs_json(self, _isolate_env): + """A stale tracked.json entry with category="cron-output" for + cron/jobs.json must NOT be deleted by quick(). + + This is the exact scenario from #37721: an old tracked.json has + {"path": ".../cron/jobs.json", "category": "cron-output"} which + would pass the delete filter but must be skipped because + guess_category() now returns None for non-output cron paths. + """ + dg = _load_lib() + cron_dir = _isolate_env / "cron" + cron_dir.mkdir() + jobs_json = cron_dir / "jobs.json" + jobs_json.write_text('{"jobs": []}') + + # Simulate a stale tracked.json entry from before #34840 by + # directly writing the tracked file (track() would reject it). + tracked_file = _isolate_env / "disk-cleanup" / "tracked.json" + tracked_file.parent.mkdir(parents=True, exist_ok=True) + tracked_file.write_text(json.dumps([{ + "path": str(jobs_json), + "category": "cron-output", + "timestamp": "2025-01-01T00:00:00+00:00", # very old + "size": 123, + }])) + + summary = dg.quick() + assert summary["deleted"] == 0, "cron/jobs.json must not be deleted" + assert jobs_json.exists(), "jobs.json must still exist" + # The stale entry should have been dropped from tracking. + remaining = json.loads(tracked_file.read_text()) + assert len(remaining) == 0 + + def test_quick_skips_stale_cron_output_for_cron_dir(self, _isolate_env): + """Stale entry for the cron/ directory itself must not be deleted.""" + dg = _load_lib() + cron_dir = _isolate_env / "cron" + cron_dir.mkdir() + output_dir = cron_dir / "output" + output_dir.mkdir() + (output_dir / "run.md").write_text("x") + + tracked_file = _isolate_env / "disk-cleanup" / "tracked.json" + tracked_file.parent.mkdir(parents=True, exist_ok=True) + tracked_file.write_text(json.dumps([{ + "path": str(cron_dir), + "category": "cron-output", + "timestamp": "2025-01-01T00:00:00+00:00", + "size": 0, + }])) + + summary = dg.quick() + assert summary["deleted"] == 0, "cron/ dir must not be deleted" + assert cron_dir.exists() + + def test_quick_skips_protected_cron_paths_defense_in_depth(self, _isolate_env): + """Defense-in-depth: even if guess_category returned cron-output + (hypothetically), protected cron paths are never deleted.""" + dg = _load_lib() + cron_dir = _isolate_env / "cron" + cron_dir.mkdir() + tick_lock = cron_dir / ".tick.lock" + tick_lock.write_text("") + + # Manually inject a stale entry with "test" category (would normally + # be auto-deleted) — the protected path guard must still block it. + tracked_file = _isolate_env / "disk-cleanup" / "tracked.json" + tracked_file.parent.mkdir(parents=True, exist_ok=True) + tracked_file.write_text(json.dumps([{ + "path": str(tick_lock), + "category": "test", + "timestamp": "2025-01-01T00:00:00+00:00", + "size": 0, + }])) + + summary = dg.quick() + assert summary["deleted"] == 0, ".tick.lock must not be deleted" + assert tick_lock.exists() + + def test_dry_run_omits_stale_cron_output(self, _isolate_env): + """dry_run() should also skip stale cron-output entries.""" + dg = _load_lib() + cron_dir = _isolate_env / "cron" + cron_dir.mkdir() + jobs_json = cron_dir / "jobs.json" + jobs_json.write_text("[]") + + tracked_file = _isolate_env / "disk-cleanup" / "tracked.json" + tracked_file.parent.mkdir(parents=True, exist_ok=True) + tracked_file.write_text(json.dumps([{ + "path": str(jobs_json), + "category": "cron-output", + "timestamp": "2025-01-01T00:00:00+00:00", + "size": 123, + }])) + + auto, prompt = dg.dry_run() + assert len(auto) == 0, "stale cron-output for jobs.json must not appear" + assert len(prompt) == 0 + + def test_legitimate_cron_output_still_deleted(self, _isolate_env): + """A valid cron-output entry under cron/output/ must still be deleted.""" + dg = _load_lib() + output_dir = _isolate_env / "cron" / "output" / "job_1" + output_dir.mkdir(parents=True) + run_md = output_dir / "run.md" + run_md.write_text("x") + + # Old enough to be deleted (>14 days) + from datetime import datetime, timezone, timedelta + old_ts = (datetime.now(timezone.utc) - timedelta(days=20)).isoformat() + + tracked_file = _isolate_env / "disk-cleanup" / "tracked.json" + tracked_file.parent.mkdir(parents=True, exist_ok=True) + tracked_file.write_text(json.dumps([{ + "path": str(run_md), + "category": "cron-output", + "timestamp": old_ts, + "size": 10, + }])) + + summary = dg.quick() + assert summary["deleted"] == 1, "valid old cron-output should be deleted" + assert not run_md.exists() + + class TestTrackForgetQuick: def test_track_then_quick_deletes_test(self, _isolate_env): dg = _load_lib()
-
9 PRs closed without merging
From the close comments: done elsewhere 3 · superseded by another PR 3 · merged via another change 2 · other 1
-
#32177 fix(plugins): preserve cron registry from disk-cleanup auto-categorisation · done elsewhere →
#32164 -
#32222 fix(disk-cleanup): restrict cron-output classification to output subtree · superseded by another PR →
#32177 -
#32436 fix(disk-cleanup): protect cron/jobs.json from deletion · superseded by another PR →
#34840,#32164,#33834 -
#32478 fix(plugins): restrict disk-cleanup cron-output classification to output/ subtree (#32164) · superseded by another PR →
#34840,#33834,#32164 -
#33834 fix(cron): exclude jobs.json registry from disk-cleanup pattern · merged via another change →
d473e7c,#34840
Show 4 more
-
#37726 fix(disk-cleanup): revalidate cron-output paths before quick() deletes them · done elsewhere →
30412a9,#37721 -
#37760 fix(cleanup): revalidate cron-output paths before deletion in quick() · done elsewhere →
30412a9,#37721,#37726 -
#37868 fix(cron): re-validate stale cron-output entries before deletion #37721 · merged via another change →
30412a9,#39067,#34840 -
#38713 fix(plugins): guard disk-cleanup against deleting cron/jobs.json (#37721) · other →
30412a9
Closed issues · 2
How this cluster was grouped
-
Link evidence (
partly structural links): some members are connected by structural evidence (shared changed files, or a closing/superseded reference); the rest are joined by an automated relevance judgement over their text. - Members outside that structural core may belong to a different underlying problem.
-
Pairwise link density
0.15: the share of item pairs that carry a link, counting a structural link in full and a relevance-only link at 0.36.1.00means every pair is linked.
Pick up a Lab task
Check whether a fix covers the open issues here, with your own agent. Bring back evidence for maintainers.
Open the Lab