diff --git a/cron/incidents.py b/cron/incidents.py index fee52bad55..623012f139 100644 --- a/cron/incidents.py +++ b/cron/incidents.py @@ -25,7 +25,9 @@ from hermes_time import now as _hermes_now # Optional test override (mirrors ``cron.executions.EXECUTIONS_FILE``). EXECUTIONS_FILE: Optional[Path] = None -INCIDENT_STATES = ("detected", "alerted", "closed") +# ``resolved``: the job ran OK after the failure (auto); ``closed``: the operator acked the signature +# and wants it silent. Only ``closed`` is terminal; a repeat of a resolved error re-opens it. +INCIDENT_STATES = ("detected", "alerted", "resolved", "closed") _FAILURE_TYPE_ORDER = ( ("rate_limit", (r"\b429\b", "rate limit", "usage limit", "quota")), ("timeout", ("timeout", "timed out")), @@ -146,7 +148,8 @@ def upsert_incident( """Record (or refresh) the incident for ``job_id`` + ``error``; returns ``(incident_id, is_new)``. An existing row for the signature refreshes ``last_seen_at``/``error``/``output_file`` and keeps its state — a ``closed`` incident stays - closed. A changed error text mints a new incident.""" + closed, while a ``resolved`` one (job recovered, then broke the same way again) re-opens as + ``detected`` so the operator is alerted once more. A changed error text mints a new incident.""" job_id = str(job_id or "") sig = _error_signature(job_id, error) stored_error = _redact_error(error) @@ -157,16 +160,19 @@ def upsert_incident( with _transaction() as conn: row = conn.execute( - "SELECT id FROM cron_incidents WHERE id=?", (incident_id,) + "SELECT id, state FROM cron_incidents WHERE id=?", (incident_id,) ).fetchone() if row is not None: + reopen = row["state"] == "resolved" conn.execute( """UPDATE cron_incidents - SET last_seen_at=?, error=?, output_file=? + SET last_seen_at=?, error=?, output_file=?, + state=CASE WHEN state='resolved' THEN 'detected' ELSE state END, + closed_at=CASE WHEN state='resolved' THEN NULL ELSE closed_at END WHERE id=?""", (now, stored_error, output_file, incident_id), ) - return incident_id, False + return incident_id, reopen conn.execute( """INSERT INTO cron_incidents (id, job_id, error_sig, state, failure_type, @@ -211,6 +217,24 @@ def ack_incident(incident_id: str) -> bool: return set_incident_state(incident_id, "closed") +def close_incidents_for_recovered_job(job_id: str) -> int: + """Mark every open incident for ``job_id`` ``resolved`` after a successful run; returns how many. + Without this the ledger only ever grows: a one-off failure (a config drift skip, a provider + outage) stayed ``detected``/``alerted`` forever after the job recovered, so ``hermes cron + incidents`` showed dozens of "open" incidents for jobs that had been green for weeks (32 of 32 on + one install). ``resolved`` is distinct from the operator's ``closed`` on purpose: a repeat of the + same error re-opens a resolved incident and alerts again (see ``upsert_incident``), whereas + ``closed`` keeps that signature silent.""" + now = _hermes_now().isoformat() + with _transaction() as conn: + cursor = conn.execute( + """UPDATE cron_incidents SET state='resolved', closed_at=? + WHERE job_id=? AND state IN ('detected', 'alerted')""", + (now, str(job_id or "")), + ) + return int(cursor.rowcount or 0) + + def _state_filter(state: Optional[str]) -> tuple[str, tuple]: return ("", ()) if state is None else (" WHERE state=?", (state,)) diff --git a/cron/scheduler.py b/cron/scheduler.py index 3d745e1360..3fa97ff156 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -347,6 +347,17 @@ def _upsert_incident_for_failure( return False, None +def _resolve_incidents_for_recovered_job(job: dict) -> None: + """Best-effort: a successful run marks the job's open incidents ``resolved`` (never touches an + operator ``closed`` ack). Store errors log at debug; delivery is unaffected.""" + try: + from cron.incidents import close_incidents_for_recovered_job + + close_incidents_for_recovered_job(job["id"]) + except Exception as exc: + logger.debug("Incident store unavailable for job %s (delivery unaffected): %s", job["id"], exc) + + def _mark_incident_alerted(incident_id: Optional[str]) -> None: """Best-effort: mark incident ``alerted`` (no-op for closed; never resurrects an acked one).""" if not incident_id: @@ -2590,6 +2601,7 @@ def _compose_run_delivery( ) elif success: deliver_content = final_response + _resolve_incidents_for_recovered_job(job) else: # Record the job+error signature once; if already acked by the operator, suppress the # per-run ping. Best-effort: a ledger failure never breaks delivery. diff --git a/hermes_cli/cron.py b/hermes_cli/cron.py index 6991fef1d2..d2258589c9 100644 --- a/hermes_cli/cron.py +++ b/hermes_cli/cron.py @@ -270,7 +270,8 @@ def cron_runs(job_id: Optional[str] = None, limit: int = 20): print(f" {record['error']}") -_INCIDENT_STATE_COLORS = {"detected": Colors.RED, "alerted": Colors.YELLOW, "closed": Colors.GREEN} +_INCIDENT_STATE_COLORS = {"detected": Colors.RED, "alerted": Colors.YELLOW, "resolved": Colors.GREEN, + "closed": Colors.DIM} def cron_incidents(args) -> int: diff --git a/hermes_cli/subcommands/cron.py b/hermes_cli/subcommands/cron.py index 2603bf57a6..f037dfd171 100644 --- a/hermes_cli/subcommands/cron.py +++ b/hermes_cli/subcommands/cron.py @@ -182,7 +182,7 @@ def build_cron_parser(subparsers, *, cmd_cron: Callable) -> None: # cron incidents — durable failure incidents (list/ack) cron_incidents = cron_subparsers.add_parser( "incidents", help="List or acknowledge durable cron failure incidents") - cron_incidents.add_argument("--state", choices=["detected", "alerted", "closed"], + cron_incidents.add_argument("--state", choices=["detected", "alerted", "resolved", "closed"], help="Filter incidents by lifecycle state") cron_incidents.add_argument( "incident_action", nargs="?", default="list", choices=["list", "ack"], diff --git a/tests/cron/test_cron_incidents_resolve.py b/tests/cron/test_cron_incidents_resolve.py new file mode 100644 index 0000000000..973cfbc0ac --- /dev/null +++ b/tests/cron/test_cron_incidents_resolve.py @@ -0,0 +1,51 @@ +"""A successful run resolves the job's open incidents; the same error afterwards re-opens them. + +The ledger only ever grew: a one-off failure (drift skip, provider outage) stayed ``detected``/``alerted`` +after the job had been green for weeks, so ``hermes cron incidents`` listed 32 "open" incidents on an +install whose every job was healthy. ``resolved`` is auto and re-openable; ``closed`` is the operator's +ack and stays silent. +""" +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).parent.parent.parent)) + +import cron.incidents as incidents +import cron.scheduler as sched + + +def _point_db(monkeypatch, tmp_path): + monkeypatch.setattr(incidents, "EXECUTIONS_FILE", tmp_path / "cron" / "executions.db") + return incidents + + +def test_successful_run_resolves_open_incidents_and_a_repeat_reopens_them(monkeypatch, tmp_path): + inc = _point_db(monkeypatch, tmp_path) + inc_id, _ = inc.upsert_incident("job-1", "provider 503") + sched._mark_incident_alerted(inc_id) + other_id, _ = inc.upsert_incident("job-2", "provider 503") + + content, *_ = sched._compose_run_delivery( + {"id": "job-1", "name": "j1"}, success=True, error=None, final_response="all good", output_file=None) + + assert content == "all good" + assert inc.get_incident(inc_id)["state"] == "resolved" + assert inc.get_incident(inc_id)["closed_at"] + assert inc.get_incident(other_id)["state"] == "detected" # another job's incident is untouched + assert inc.count_incidents("resolved") == 1 + + same_id, reopened = inc.upsert_incident("job-1", "provider 503") + assert same_id == inc_id and reopened is True + assert inc.get_incident(inc_id)["state"] == "detected" + assert inc.get_incident(inc_id)["closed_at"] is None + + +def test_operator_ack_survives_a_successful_run(monkeypatch, tmp_path): + inc = _point_db(monkeypatch, tmp_path) + inc_id, _ = inc.upsert_incident("job-1", "known flaky") + inc.ack_incident(inc_id) + + assert inc.close_incidents_for_recovered_job("job-1") == 0 + assert inc.get_incident(inc_id)["state"] == "closed" + _, reopened = inc.upsert_incident("job-1", "known flaky") + assert reopened is False and inc.get_incident(inc_id)["state"] == "closed" diff --git a/website/docs/user-guide/features/cron.md b/website/docs/user-guide/features/cron.md index b9634de658..73d96cdd54 100644 --- a/website/docs/user-guide/features/cron.md +++ b/website/docs/user-guide/features/cron.md @@ -429,7 +429,7 @@ ledger database as the execution history. ```bash hermes cron incidents # list incidents (newest activity first) -hermes cron incidents --state alerted # filter: detected | alerted | closed +hermes cron incidents --state alerted # filter: detected | alerted | resolved | closed hermes cron incidents ack # acknowledge — stop re-pinging ``` @@ -437,11 +437,17 @@ Acknowledging an incident silences the per-run failure ping for that exact signature only. Nothing else changes: the run history still records every failure, the failure streak keeps counting, and the moment the job starts failing with a *different* error a new incident is minted and alerts fire -again. A successful run doesn't touch incidents — they are per-signature, not -per-job. +again. + +A successful run marks every open incident for that job `resolved`, so the +list reflects current health rather than every failure the job ever had. If +the job later fails with the *same* error, the resolved incident re-opens as +`detected` and you are alerted again. Acknowledged (`closed`) incidents are +the exception: a success leaves them alone, and a repeat stays silent. Incident lifecycle: `detected` (failure recorded) → `alerted` (at least one -failure ping reached delivery) → `closed` (acknowledged; terminal for that +failure ping reached delivery) → `resolved` (the job ran OK afterwards; +re-opens on a repeat) or `closed` (acknowledged; terminal for that signature). Stored error text is secret-redacted and truncated before it is written.