Skip to content

fix(cron): distinguish and clean up script timeouts - #62942

Closed
rooroosu wants to merge 1 commit into
NousResearch:mainfrom
rooroosu:fix/cron-script-timeout-reporting
Closed

rooroosu wants to merge 1 commit into
NousResearch:mainfrom
rooroosu:fix/cron-script-timeout-reporting

Conversation

@rooroosu

Copy link
Copy Markdown

Summary

  • classify no-agent/script deadlines as script timeouts instead of provider failures
  • terminate timed-out POSIX script process groups so descendants do not survive
  • preserve provider-timeout summaries for true agent/provider failures

Root cause

_summarize_cron_failure_for_delivery treated any error containing timeout as a provider timeout, even when _run_job_script emitted Script timed out after ... for a no-agent job. The script runner also used subprocess.run(timeout=...), which could leave grandchildren alive after the direct child was killed.

Verification

  • scripts/run_tests.sh tests/cron/test_cron_script.py tests/cron/test_run_one_job.py tests/cron/test_cron_no_agent.py -q
  • 69 tests passed
  • git diff --check

Platform behavior

POSIX uses a dedicated session/process group with TERM then KILL fallback. Windows retains compatible direct-child cleanup.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused cron cleanup. The premise is confirmed on current origin/main: cron/scheduler.py:75-79 classifies the script runner's Script timed out ... result (cron/scheduler.py:2132-2133) as a provider timeout, and the runner uses a direct-child subprocess.run(..., timeout=...) path at cron/scheduler.py:2100-2108.

Problems

  • tests/cron/test_cron_script.py:230 uses os.kill(child_pid, 0) to require the child PID to disappear. A successfully signaled, reparented descendant may remain an unreaped zombie, so this can fail after correct cleanup. Existing process-tree coverage explicitly treats dead-or-zombie as terminated in tests/tools/test_process_registry.py:1928-1952.

Suggested changes

  • Make the new descendant assertion zombie-aware, following the existing process-tree test pattern; retain the POSIX-only marker and cleanup guard.

Automated hermes-sweeper review.

deadline = time.monotonic() + 5
while time.monotonic() < deadline:
try:
os.kill(child_pid, 0)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.kill(pid, 0) also succeeds for an unreaped zombie, so this can fail after successful killpg cleanup when the reparented child has not yet been reaped. Please use a zombie-aware probe, following tests/tools/test_process_registry.py:1928-1952.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists labels Jul 12, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Duplicate of #59379 — both fix #59549 with the same two-part change in cron/scheduler.py: check "script timed out" before the generic provider-timeout branch in _summarize_cron_failure_for_delivery, and run the script in its own process group so a timeout kills descendants via os.killpg. #59379 (2026-07-06 04:46) is the earliest-open full-scope fix. Related: #59574 (competing full-scope), #61969 (classification-only subset), #59549 (issue).

@teknium1 teknium1 added 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 12, 2026
@rooroosu rooroosu closed this Jul 19, 2026
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 duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists 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-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants