diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 8f63270a0ec6..df32877052d5 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -9567,6 +9567,29 @@ def _record_task_failure( if force_trip or failures >= effective_limit: # Trip the breaker. + # + # Persist a FLOOR of ``effective_limit`` on the stored counter, + # not the raw ``failures`` count. ``force_trip=True`` callers + # (protocol-violation streak, systemic crash fingerprint) trip + # on a threshold that has nothing to do with the unified + # ``consecutive_failures`` column — e.g. a violation streak of + # 3 force-trips with ``failure_limit=3`` while the column itself + # sat at 0 (reset by an earlier unblock), so ``failures`` here is + # only 1. Storing that raw ``1`` let ``recompute_ready`` — which + # runs later in the SAME dispatch tick and re-resolves its own + # (unrelated, often lower) effective_limit — see "1 < 2" and + # immediately promote the task straight back to ``ready``, + # undoing the trip within the same tick. Observed as an infinite + # blocked -> ready -> crash loop for zero-parent tasks (surfaced + # as `{"trigger": "parents_terminal", "satisfied_parent_ids": + # []}` since a zero-parent task's parent list is vacuously + # "all done"); t_d1994b5b hit this 96+ times. Flooring the + # stored value at the limit that actually tripped it makes any + # later re-check computing the same-or-lower limit correctly see + # the task as still over threshold; a genuinely higher configured + # limit can still recover it (unified counter semantics + # preserved, just no longer allowed to under-report). + stored_failures = max(failures, effective_limit) if release_claim: # Spawn path: still running, also clear claim state. conn.execute( @@ -9574,7 +9597,7 @@ def _record_task_failure( "claim_expires = NULL, worker_pid = NULL, " "consecutive_failures = ?, last_failure_error = ? " "WHERE id = ? AND status IN ('running', 'ready')", - (failures, error[:500], task_id), + (stored_failures, error[:500], task_id), ) else: # Timeout/crash path: task is already at ``ready`` @@ -9584,7 +9607,7 @@ def _record_task_failure( "UPDATE tasks SET status = 'blocked', " "consecutive_failures = ?, last_failure_error = ? " "WHERE id = ? AND status IN ('ready', 'running')", - (failures, error[:500], task_id), + (stored_failures, error[:500], task_id), ) run_id = None if end_run: diff --git a/tests/hermes_cli/test_kanban_blocked_sticky.py b/tests/hermes_cli/test_kanban_blocked_sticky.py index 9c3e4f4b89f2..1dfaeee09a9e 100644 --- a/tests/hermes_cli/test_kanban_blocked_sticky.py +++ b/tests/hermes_cli/test_kanban_blocked_sticky.py @@ -161,3 +161,70 @@ def test_protocol_violation_loop_is_broken(kanban_home: Path) -> None: # (landed via #28754 / #28781). The original PR shipped a duplicate test # here; dropped during salvage to avoid two assertions of the same contract. # --------------------------------------------------------------------------- + + +# --------------------------------------------------------------------------- +# consecutive_failures must be floored at the tripping effective_limit +# (t_21f59f6d) — otherwise a same-tick re-resolution of a lower/unrelated +# effective_limit under-reads the counter and undoes the trip immediately. +# --------------------------------------------------------------------------- + + +def test_force_trip_floors_consecutive_failures_at_effective_limit( + kanban_home: Path, +) -> None: + """``_record_task_failure(force_trip=True)`` must persist + ``max(failures, effective_limit)`` on ``consecutive_failures``, not the + raw per-call ``failures`` count. + + Bug shape (t_21f59f6d / t_d1994b5b / t_342c4c9f): a force-trip caller + (e.g. the protocol-violation streak) trips the breaker on a threshold + unrelated to the unified ``consecutive_failures`` column. If the column + was reset to 0 by a prior unblock, the very first force-trip call + computes ``failures=1`` even though it is tripping on, say, + ``failure_limit=3``. Storing that raw ``1`` let ``recompute_ready`` — + invoked later in the SAME dispatch tick with its own (often lower) + resolved ``effective_limit`` — see ``1 < effective_limit`` and + immediately promote the task back to ``ready``, undoing the trip and + producing an infinite blocked -> ready -> crash loop surfaced as a + ``promoted`` event with ``trigger=parents_terminal`` and + ``satisfied_parent_ids=[]`` on a zero-parent task. + """ + with kb.connect() as conn: + tid = kb.create_task(conn, title="force-trip floor reproducer") + kb.claim_task(conn, tid) + + # Force-trip with a higher threshold than the raw per-call + # ``failures`` count (starts at 0 -> 1 on this first call). + tripped = kb._record_task_failure( + conn, tid, + error="protocol violation streak", + outcome="crashed", + failure_limit=3, + force_trip=True, + release_claim=True, + end_run=True, + ) + assert tripped is True + task = kb.get_task(conn, tid) + assert task.status == "blocked" + # The critical assertion: the stored counter must be floored at + # the effective_limit that tripped the breaker (3), not the raw + # per-call failures count (1). + assert task.consecutive_failures == 3, ( + "consecutive_failures must be floored at effective_limit " + f"(3), got {task.consecutive_failures} — under-reporting " + "lets a later lower-limit recompute_ready call re-promote " + "the task within the same dispatch tick" + ) + + # Simulate the SAME dispatch tick re-resolving a lower/unrelated + # default failure_limit via recompute_ready. Before the fix, the + # under-reported counter (1) would satisfy `1 < 2` and promote + # the task straight back to ready. + promoted = kb.recompute_ready(conn, failure_limit=2) + assert promoted == 0, ( + "task must NOT be re-promoted within the same tick when its " + "floored failure count still exceeds a lower effective_limit" + ) + assert kb.get_task(conn, tid).status == "blocked"