From dd03471858a1fb00c3fa0a62bb6bd4ec8b18e7f5 Mon Sep 17 00:00:00 2001 From: Jack Lau <72348727+jackulau@users.noreply.github.com> Date: Thu, 20 Aug 2026 03:52:44 -0500 Subject: [PATCH] fix(cron): nudge review of escaped-run failures too A recurring job that fails at the scheduler layer - an exception escaping run_one_job's body before the agent is ever constructed - has delivered a failure alert since 4668750fa. It has never carried the repeated-failure review nudge the normal agent-failure delivery carries: the nudge (#80752, 2026-08-06) predates that second delivery site by eight days and only ever composed the first one. The streak itself is layer-agnostic. mark_job_run increments failure_streak for an escaped failure exactly as it does for an agent failure, and the escape handler calls it. So the counter climbs correctly and shows up in `hermes cron list`, but the chat message that spends it is unreachable for a job whose failures ALL escape - a half-applied update leaving a bad import, a provider client that cannot construct. Those are precisely the failures that repeat identically on every tick, so the operator gets the same one-line error every 10 minutes indefinitely and is never told the automation itself is worth reviewing or pausing. Compose the nudge at the escape handler's delivery exactly as the normal path does. It stays config-gated and threshold-gated by the same helper, so a first-time escaped failure reads exactly as it did before. Docs said the streak counts "runs where the agent failed", which is what the reporter read and reasonably concluded their failures were out of scope. The counter never worked that way; correct the sentence to match the code. Tests: two cases on the escaped-failure delivery path - streak at threshold appends the nudge (fails on the unfixed handler with the bare summary), and streak below threshold delivers the unchanged one-liner, so the guard also proves the nudge is not unconditional. The existing nudge tests only ever exercised the helper in isolation, which is why the second delivery site could be added without it. Fixes #88655 --- cron/scheduler.py | 9 ++- tests/cron/test_run_one_job.py | 76 ++++++++++++++++++++++++ website/docs/user-guide/features/cron.md | 16 ++--- 3 files changed, 93 insertions(+), 8 deletions(-) diff --git a/cron/scheduler.py b/cron/scheduler.py index 082cdaedd1..19cbcbc537 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -7070,7 +7070,14 @@ def _run_one_job_body( delivery_attempted = True delivery_error = _deliver_result( job, - _summarize_cron_failure_for_delivery(job, _err_text), + # Composed exactly like the normal failure delivery above. + # mark_job_run below records THIS run in failure_streak + # whichever layer failed, so a job that fails before the + # run body every tick builds a streak nobody is ever told + # about: its alerts only ever leave through here, and the + # nudge only ever left through there (#88655). + _summarize_cron_failure_for_delivery(job, _err_text) + + _failure_streak_nudge(job), adapters=adapters, loop=loop, ) diff --git a/tests/cron/test_run_one_job.py b/tests/cron/test_run_one_job.py index 190d6049c8..6d370b9ff0 100644 --- a/tests/cron/test_run_one_job.py +++ b/tests/cron/test_run_one_job.py @@ -161,6 +161,82 @@ def test_run_one_job_exception_records_failure_alert_delivery_error(monkeypatch) ] +def _patch_escaped_failure(monkeypatch, delivered, *, exec_id, err): + """Make run_job raise, and capture what the escape handler delivers.""" + monkeypatch.setattr(s, "create_execution", lambda *_a, **_kw: {"id": exec_id}) + monkeypatch.setattr(s, "claim_dispatch", lambda _job_id: True) + monkeypatch.setattr(s, "mark_execution_running", lambda _execution_id: None) + monkeypatch.setattr( + s, + "run_job", + lambda *_a, **_kw: (_ for _ in ()).throw(RuntimeError(err)), + ) + monkeypatch.setattr( + s, + "_deliver_result", + lambda job, content, **_kw: delivered.append(content) or None, + ) + monkeypatch.setattr(s, "mark_job_run", lambda *_a, **_kw: None) + monkeypatch.setattr(s, "finish_execution", lambda *_a, **_kw: None) + # Deterministic threshold: default 3, independent of the host config. + monkeypatch.setattr(s, "load_config", lambda: {}) + + +def test_escaped_failure_delivery_carries_the_streak_nudge(monkeypatch): + """A repeatedly-failing job must be nudged even when it fails at the + scheduler layer (#88655). + + ``mark_job_run`` increments ``failure_streak`` for an escaped failure just + as it does for an agent failure, so the counter climbs either way. But the + nudge that spends it was only composed on the normal delivery path, so a + job that raises before the run body on every tick - a bad import from a + half-applied update, a provider client that cannot construct - alerts + forever and is never told it should be reviewed or paused. Nothing else + surfaces the streak in chat. + """ + delivered = [] + _patch_escaped_failure( + monkeypatch, delivered, exec_id="exec-j5", err="cannot import name X" + ) + + ok = s.run_one_job( + { + "id": "j5", + "name": "scout", + "deliver": "telegram", + "schedule": {"kind": "interval"}, + "failure_streak": 2, # + this run = 3 = default threshold + } + ) + + assert ok is False + assert len(delivered) == 1 + assert "cannot import name X" in delivered[0] + assert "failed 3 runs in a row" in delivered[0] + assert "hermes cron pause scout" in delivered[0] + + +def test_escaped_failure_delivery_stays_quiet_below_the_threshold(monkeypatch): + """The nudge is appended, not always-on: a first failure reads as before.""" + delivered = [] + _patch_escaped_failure( + monkeypatch, delivered, exec_id="exec-j6", err="provider failed" + ) + + ok = s.run_one_job( + { + "id": "j6", + "name": "scout", + "deliver": "telegram", + "schedule": {"kind": "interval"}, + "failure_streak": 0, + } + ) + + assert ok is False + assert delivered == ["⚠️ Cron 'scout' failed: provider failed"] + + def test_run_one_job_exception_after_delivery_does_not_redeliver(monkeypatch): """Once delivery has been attempted, the outer handler must not send again.""" delivered = [] diff --git a/website/docs/user-guide/features/cron.md b/website/docs/user-guide/features/cron.md index ab1e185f14..cb91709320 100644 --- a/website/docs/user-guide/features/cron.md +++ b/website/docs/user-guide/features/cron.md @@ -334,13 +334,15 @@ ledger is included in quick backups. ### Repeated-failure review nudge -Each job tracks a `failure_streak` — consecutive runs where the agent failed -(delivery failures don't count). When a *recurring* job's streak reaches the -threshold, the failure message delivered to chat gains a review nudge telling -you the job has failed N runs in a row and suggesting you fix, pause -(`hermes cron pause `), or remove it. Any successful run resets the -streak, and `hermes cron list` shows the streak alongside a failing job's last -run. One-shot jobs never nudge. +Each job tracks a `failure_streak` — consecutive failed runs (delivery +failures don't count). A run that fails before the agent is reached at all — +a bad import after a half-applied update, a provider client that cannot be +constructed — counts and alerts the same as one the agent itself failed. When +a *recurring* job's streak reaches the threshold, the failure message +delivered to chat gains a review nudge telling you the job has failed N runs +in a row and suggesting you fix, pause (`hermes cron pause `), or remove +it. Any successful run resets the streak, and `hermes cron list` shows the +streak alongside a failing job's last run. One-shot jobs never nudge. ```yaml cron: