Skip to content

fix(cron): keep active manual runs heartbeating - #92018

Open
qdivan wants to merge 1 commit into
NousResearch:mainfrom
qdivan:fix/cron-run-heartbeat-ceiling
Open

fix(cron): keep active manual runs heartbeating#92018
qdivan wants to merge 1 commit into
NousResearch:mainfrom
qdivan:fix/cron-run-heartbeat-ceiling

Conversation

@qdivan

@qdivan qdivan commented Aug 22, 2026

Copy link
Copy Markdown

What does this PR do?

Keeps the calling turn's activity heartbeat alive for the full duration of an inline manual cron run.

The heartbeat currently stops after a hard-coded six-hour ceiling even when run_one_job is still active. Once it stops, the parent inactivity watchdog can terminate a legitimate long-running job at a deterministic wall-clock threshold. Elapsed time alone is not evidence that the child is wedged: persistent gateway sessions already dispatch manual runs through the async-delegation rail, while synchronous fallbacks remain governed by the cron run's own timeout/cancellation policy (including an explicit unlimited setting).

This removes the independent heartbeat ceiling and leaves the heartbeat tied to the actual run_one_job lifetime. The existing finally block still stops and joins the heartbeat thread as soon as the job exits.

Related Issue

No linked issue; this is a self-found regression in the manual-run heartbeat introduced by #76675. It is distinct from the opt-in per-job caps proposed in #45809.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✅ Tests (adding or improving test coverage)

Changes Made

  • tools/cronjob_tools.py: remove the elapsed-time gate that drops activity while an inline cron run is still executing.
  • tests/tools/test_cronjob_run_immediate.py: replace the old ceiling contract with a deterministic monotonic-clock regression proving heartbeats continue beyond six hours until completion.

How to Test

  1. Run scripts/run_tests.sh tests/tools/test_cronjob_run_immediate.py tests/tools/test_cronjob_run_background.py tests/tools/test_cronjob_tools.py -q (94 passed).
  2. Run scripts/run_tests.sh tests/cron/ -q (873 passed, 1 Windows-only test skipped on Linux).
  3. Run uv run --with ruff ruff check tools/cronjob_tools.py tests/tools/test_cronjob_run_immediate.py.

The focused regression was verified RED before the production change: the old code logged that the ceiling was reached, delivered no heartbeat, and failed while the fake job remained active. It passes after this change.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • 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 entire tests/ suite (focused tool and cron suites above are green)
  • I've added tests for my changes
  • I've tested on Linux

Documentation & Housekeeping

  • Documentation update — N/A; no user-facing contract or config changed
  • cli-config.yaml.example update — N/A; no config keys changed
  • CONTRIBUTING.md / AGENTS.md update — N/A; no workflow changed
  • Cross-platform impact considered — stdlib threading/time behavior is platform-neutral
  • Tool descriptions/schemas update — N/A; tool schema is unchanged

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management labels Aug 22, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

  • tools/cronjob_tools.py:835-845 — removing the ceiling entirely resurrects the pre-[Bug]: cronjob action='run' blocks the calling turn synchronously — inactivity watchdog kills the parent agent at 1800s #76502 hazard its comment documented: the heartbeat proves the thread is alive, not that the job is progressing, so a wedged run_one_job under HERMES_CRON_TIMEOUT=0 now masks the gateway watchdog forever (the new test literally pins a sleeping job being heartbeated). Please reintroduce a backstop — a much higher hard cap, or a progress-aware signal (e.g., stop heartbeating if no child-side activity marker advances for N intervals) — or document why unlimited masking is now acceptable.
  • tools/cronjob_tools.py:27-31 (removed comment) — operators who currently depend on the 6h reap to clean up hung manual runs will see turns that never die after this lands; this belongs in the changelog with the recommended mitigation ("set HERMES_CRON_TIMEOUT instead of relying on the ceiling").
  • tests/tools/test_cronjob_run_immediate.py:231-262 — the flipped test cleanly pins the new contract (elapsed time alone must not drop protection); add its missing counterpart once a backstop exists: a wedged job with unlimited child timeout still gets reaped by something, and the old "heartbeat stops at ceiling" path has a test asserting whatever replaces it.
  • nit: with the ceiling gone, the loop's logger.warning import/usage may be unused now — check lint; also consider logging at INFO every hour while heartbeating so ultra-long legitimate runs remain visible in logs.

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

Labels

comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants