Repository navigation
Conversation
…p before classifying A worker that exits rc=0 with zero heartbeats (routing/credential failure before the model ever ran) was classified as a protocol violation and counted against the protocol-violation streak. It never made a single MCP tool call, so it never had the chance to do or skip the work. _classify_dead_worker now takes the last-heartbeat timestamp and elapsed time; a clean exit with no heartbeat ever recorded (after the crash grace window) is booked as a route_failure event with its own bounded streak (_ROUTE_FAILURE_FAILURE_LIMIT=2), independent of the protocol-violation budget, and is skipped (not broken) by the protocol-violation streak walk. _reclaim_dead_workers now reaps zombies before reading the running-task rows, and reads each run's last_heartbeat_at before _end_run overwrites it, so a child that exits between two reaper passes is classified from its real exit status instead of falling into the unknown/'pid not alive' registry-miss branch. Both clean-exit branches (route_failure and protocol_violation) now stamp reap_status=observed so the two exit-status races are countable; the registry-miss branch stamps reap_status=registry_miss. Tests: test_route_failure_does_not_consume_protocol_violation_streak, test_two_trailing_route_failures_trip_the_breaker, test_reap_ordering_classifies_real_exit_status_not_unknown, test_route_failure_respects_crash_grace_window, test_reap_status_recorded_on_both_clean_exit_branches. (cherry picked from commit 507d7beff1562a86d73f5e13407621656629e98e)
…tests
Replace the three hardcoded "hermes01:mock" literals in
test_route_failure_respects_crash_grace_window and
test_reap_status_recorded_on_both_clean_exit_branches with the same
runtime host-prefix derivation the sibling tests in this file already
use: kb._claimer_id().split(":", 1)[0]. No assertion or production
code changes.
gaoanze888
left a comment
There was a problem hiding this comment.
The reap-before-classify ordering and separation from the protocol-violation streak are sensible, but the exact head 87f00c0d5820342710523b43a138b6b295ec28bc currently fails a required cross-platform test gate.
test_reap_ordering_classifies_real_exit_status_not_unknown unconditionally executes POSIX true and reads Linux-only /proc/<pid>/stat without a Linux-only marker. On macOS the focused run is 4 passed, 1 failed, because /proc is absent and the asserted zombie state remains None. Mark this test Linux-only under the repository convention, or replace the /proc dependency with a portable synchronization/exit-status fixture. git diff --check also reports a new blank line at EOF in the same test file.
Two contract gaps should be clarified and tested:
- Route failures use a fixed limit of two rather than the task's
max_retries; therefore a task configured withmax_retries=1still gets two route attempts. If this independent budget is intentional, document the override explicitly and test it. - Current tests inject a wait status; add a lightweight real worker/CLI path showing the key premise that a routing/credential failure exits rc=0 without a turn-start heartbeat.
Please also update the English/Chinese Kanban event documentation for the new public route_failure event and the fact that route failures do not reset/consume the protocol-violation streak. Ruff is clean.
…trailing blank line Review on PR NousResearch#109468 at 87f00c0 flagged two cross-platform gate failures: - test_reap_ordering_classifies_real_exit_status_not_unknown spawns a real child and reads Linux-only /proc/<pid>/stat to observe its zombie state, unconditionally. On macOS /proc is absent and the test fails (4 passed, 1 failed in the focused run). Mark it with the repository's existing pytest.mark.linux_only convention (see pyproject.toml markers and its many uses under tests/hermes_cli/). - git diff --check flagged a new blank line at EOF in the same file. git diff --check 1c671be HEAD -- tests/hermes_cli/test_kanban_core_functionality.py is now clean (was: 'new blank line at EOF').
Review on PR NousResearch#109468 raised two contract gaps: - Route failures use a fixed limit of two (_ROUTE_FAILURE_FAILURE_LIMIT) rather than the task's max_retries, so a task configured with max_retries=1 still gets two route attempts. This is DELIBERATE: a route failure means the worker never reached the model at all, so a task's retry budget for its own agent work has no bearing on how many times to re-attempt routing/credentials before surfacing a persistently unreachable route. Documented explicitly in the code comment above _ROUTE_FAILURE_FAILURE_LIMIT and pinned by test_route_failure_budget_ignores_task_max_retries (a task with max_retries=1 still gets exactly two route-failure attempts before the breaker trips). Chose 'document + test' over 'read max_retries' because the two budgets model different things (agent-work retries vs. routing/credential retries) and coupling them would be the larger, riskier change the review did not ask for. - Existing route-failure tests inject a wait status via _record_worker_exit rather than exercising a real process exit. Added test_route_failure_classified_from_real_worker_exit: spawns a genuine subprocess (sys.executable -c 'pass', portable — no POSIX-only command), lets the REAL reap_worker_zombies()/os.waitpid path reap it with no wait-status injection or monkeypatching, and asserts detect_crashed_workers classifies it as a route_failure. Needs no Linux-only marker: the process is fully reaped (not a lingering zombie) before the liveness check runs, so no platform-specific zombie-state probe is required.
|
All four points are answered in three new commits on the branch:
Each new test fails when its corresponding fix is reverted, and the focused selection is green ( Thanks for the careful review. The Windows probe and process-identity points you raised on the respawn-guard PR are being handled in that branch separately. |
# Conflicts: # hermes_cli/kanban_db_dispatch.py
Task specified merging be67e1d, but upstream main advanced further to 8e11666 (248 commits ahead of our prior merge base 416a817) by the time this task ran; merged the actual current origin/main tip instead of a stale target, per the task's own known-limitations note that a moved main makes the merge stale on landing. Resolved conflict in hermes_cli/kanban_db_dispatch.py::_run_reclaim_phase: kept both our comment (reap_worker_zombies moved into _reclaim_dead_workers) and upstream's new reap_terminal_workers() call plus its result assignment.
…sifier-20260913b # Conflicts: # hermes_cli/kanban_db_dispatch.py # tests/hermes_cli/test_kanban_core_functionality.py
…sifier-20260913b # Conflicts: # hermes_cli/kanban_db_dispatch.py
|
Docs ask is addressed in One accuracy note so the docs match the code: the marker is not a new event name in |
|
Correction to my previous comment, and thank you for the phrasing that was right all along. A route failure does emit its own Both the English reference and its Chinese translation are corrected in |
…sifier-20260913b # Conflicts: # tests/hermes_cli/test_kanban_core_functionality.py
The retired linux_only marker is rejected at collection on current main (tests/_fixtures/platform_gating.py). Local refresh of PR NousResearch#109468.
…assifier-20260913b
What breaks: a worker that exits rc=0 having made no tool call at all, the shape a routing or credential failure takes before the model ever runs, is booked as a protocol violation and counted against the protocol-violation streak. With the common per-task
max_retries=1that single misclassification blocks the card.Repro: a card whose worker dies with zero heartbeats is closed as
crashedwithpid <n> not aliveand counted against_PROTOCOL_VIOLATION_FAILURE_LIMIT.Fix, three parts:
_classify_dead_workertakes the closing run's last heartbeat and elapsed time and classifies a clean exit with no heartbeat past the crash grace asroute_failure;_reclaim_dead_workersreaps the zombie before classifying so a child exiting between the two calls is classified from its real exit status; route failures do not consume the protocol-violation streak and carry their own budget of two trailing failures before the breaker trips.Adjacent work:
#107886classifies 429/quota exits ascapacity_failurein the same function. Ours is the zero-turn rc=0 case; the labels stay distinct and the ordering between the two branches is explicit.Tests: five invariant tests, each failing on the base commit, including the ordering classification, streak non-consumption, and the breaker payload.