Skip to content

fix(kanban): make worker termination and retries fail closed - #69942

Draft
vutruongduc wants to merge 6 commits into
NousResearch:mainfrom
vutruongduc:devops/kanban-worker-lifecycle
Draft

vutruongduc wants to merge 6 commits into
NousResearch:mainfrom
vutruongduc:devops/kanban-worker-lifecycle

Conversation

@vutruongduc

@vutruongduc vutruongduc commented Jul 23, 2026 •

Copy link
Copy Markdown

What does this PR do?

This fixes a Kanban worker incident in which a worker successfully called a terminal task tool, continued through its remaining model iterations, and left its task lease active. The same run was then reported as a timeout with max_runtime=0s, even though the actual trigger was iteration-budget exhaustion.

The root causes were split across the worker lifecycle: terminal task transitions were handled like ordinary tool calls, ownership was not fenced at every API/tool boundary, process cleanup targeted a PID instead of the whole worker process group, and the per-task retry policy was not persisted and reported end to end. A refunded execute_code call could also make the cleanup-reserve counter disagree with the hard API-call limit, while a persisted local-shell snapshot could restore an older budget into later tool calls.

The fix makes worker shutdown fail closed while preserving message-role alternation and existing non-worker behavior.

Related Issue

No public issue. This was reproduced from an operator incident in a scheduled Kanban workflow.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests
  • ♻️ Refactor
  • 🎯 New skill

Changes Made

  • Treat successful kanban_complete and kanban_block calls as terminal barriers: skip later calls in the batch with paired tool results and do not make another model call. A failed terminal transition still lets the worker continue.
  • Revalidate task/run ownership before each worker API iteration and tool call using the task status, current run, claim token and expiry, and registered PID.
  • Close the dispatcher PID-registration race and terminate the full POSIX worker process group on reclaim, external block, runtime expiry, and stale ownership.
  • Distinguish iteration_exhausted from wall-clock timed_out, preserve effective budget metadata, and avoid misleading max_runtime=0s notifications.
  • Cap the cleanup reserve with both counters: max(0, min(iteration_budget.remaining, max_iterations - api_call_count)). This prevents a refunded execute_code call from bypassing the hard API limit.
  • Refresh Kanban iteration variables from the parent process for every local terminal invocation, exclude them from persisted shell snapshots, and make direct shell assignment/export unable to replace the current values.
  • Allocate snapshot writer temp files with mktemp, which keeps concurrent snapshot updates atomic on macOS Bash 3.2 where BASHPID is empty.
  • Add strict per-task max_retries >= 1 persistence and readback. max_retries=1 blocks on the first abnormal outcome without an automatic retry.
  • Add behavior tests for terminal barriers, ownership loss, process descendants, retry limits, effective iteration accounting, shell snapshot refresh, direct override attempts, concurrent snapshot writers, diagnostics, and notifier text.

How to Test

Run the directly related lifecycle and environment suite with file retries disabled:

HERMES_PYTHON=/path/to/python scripts/run_tests.sh --file-retries 0 -j6 tests/agent/test_kanban_stop.py tests/agent/test_turn_finalizer_iteration_limit_exit.py tests/gateway/test_kanban_notifier.py tests/hermes_cli/test_kanban_core_functionality.py tests/hermes_cli/test_kanban_diagnostics.py tests/hermes_cli/test_kanban_notify.py tests/run_agent/test_run_agent.py tests/run_agent/test_tool_batch_segmentation.py tests/tools/test_kanban_tools.py tests/tools/test_base_environment.py tests/tools/test_local_env_blocklist.py tests/tools/test_local_env_session_leak.py tests/tools/test_local_shell_init.py tests/tools/test_terminal_env_bridge.py tests/tools/test_local_env_iteration_budget.py

Stable focused regressions:

  • tests/tools/test_local_env_iteration_budget.py::test_kanban_budget_refreshes_after_snapshot
  • tests/tools/test_base_environment.py::TestAtomicSnapshotWrite::test_temp_path_is_allocated_atomically_with_mktemp

Observed results on macOS:

  • Combined related suite: 1005 passed across 15 files, 0 failed, file retries disabled.
  • Core iteration-accounting files: 464 passed, 0 failed, file retries disabled.
  • Environment files: 118 passed, 0 failed, file retries disabled.
  • Snapshot concurrency file: 33 passed in three consecutive runs, 0 failed, file retries disabled.
  • git diff --check: clean.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run the full test suite; the focused 1005-test suite above passes
  • I've added tests for my changes
  • I've tested on macOS

Documentation & Housekeeping

  • Documentation update is N/A; CLI help, schema descriptions, config comments, diagnostics, and localized notifier text are updated in place
  • cli-config.yaml.example update is N/A; no new global config key is added
  • CONTRIBUTING.md and AGENTS.md updates are N/A
  • Cross-platform process semantics need maintainer review; process-group termination is POSIX-gated
  • Tool descriptions and schemas are updated for max_retries

Additional hook-consent regression

The rollout canary also exposed that an explicit top-level --accept-hooks flag was ignored by hermes hooks list and hermes hooks doctor because management commands intentionally skip full agent startup. The CLI now registers configured hooks only when this explicit flag is present; ordinary hook inspection remains non-mutating and mtime-drift protection is unchanged.

Focused hook and parser validation: 117 tests passed across 4 files, 0 failed.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/gateway Gateway runner, session dispatch, delivery comp/cron Cron scheduler and job management needs-decision Awaiting maintainer decision before any implementation sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 23, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related to #69036 (worker-group reclamation) and #63637 (runtime ownership). This PR also changes terminal tool barriers and iteration-exhaustion/notifier semantics, so it is not a duplicate; please choose whether to consolidate the overlapping lifecycle work.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the extensive worker-lifecycle hardening. The core premise remains live on current main: agent/tool_dispatch_helpers.py:43 has no Kanban terminal barrier; agent/turn_finalizer.py:151-170 records iteration exhaustion as timed_out; and hermes_cli/kanban_db.py:6894-6928 signals only a worker PID despite start_new_session=True at :8967-8976.

Problems

  • The max_retries portion overlaps current-main functionality already introduced by ac51c4c1ad; current model/storage support is at hermes_cli/kanban_db.py:918-925. Those old-base hunks require reconciliation rather than direct application.
  • The hook-consent change is unrelated to the Kanban lifecycle scope and should be reviewed independently.

Suggested changes

  • Salvage the terminal-barrier, ownership, process-group, and iteration-outcome fixes against current main, while retaining the existing retry implementation.
  • Split the hook management-command change into a focused follow-up.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants