Skip to content

fix(cron): classify local script timeouts separately - #61969

Closed
angroni84-wq wants to merge 1 commit into
NousResearch:mainfrom
angroni84-wq:fix/cron-script-timeout-wording-current
Closed

fix(cron): classify local script timeouts separately#61969
angroni84-wq wants to merge 1 commit into
NousResearch:mainfrom
angroni84-wq:fix/cron-script-timeout-wording-current

Conversation

@angroni84-wq

Copy link
Copy Markdown

What does this PR do?

Cron delivery summaries can misclassify a local pre-run script timeout as a provider failure. _run_job_script() emits an exact envelope such as Script timed out after 120s: ..., but broad keyword checks for 429, rate limit, and timeout can shadow the real cause when those terms appear in the script path or error text.

This change classifies the exact local-script timeout envelope before provider/API keyword checks. Provider timeout and rate-limit behavior remains unchanged for genuine provider failures.

No matching open/closed issue or pull request was found in the pre-submission search.

Related Issue

No linked issue.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • cron/scheduler.py: classify the exact local script-timeout envelope before broad provider-keyword checks.
  • tests/cron/test_scheduler_failure_summary.py: add precedence and sanitization regressions for integer/decimal local timeouts, paths containing 429/rate-limit, genuine provider timeouts, and generic script failures.
  • tests/cron/test_cron_script.py: add an integration regression from _run_job_script() timeout output through the delivery-summary classifier.

How to Test

  1. Run python -m pytest -o 'addopts=' -q tests/cron/test_scheduler_failure_summary.py.
  2. Run python -m pytest -o 'addopts=' -q tests/cron/test_cron_script.py.
  3. Run python -m pytest -o 'addopts=' -q tests/cron with an isolated process-level HOME, HERMES_HOME, XDG config/cache paths, and TMPDIR.

Local verification on macOS:

  • Failure-summary tests: 7 passed
  • Script tests: 39 passed
  • Complete cron suite: 678 passed, 2 warnings
  • py_compile: passed
  • git diff --check: passed
  • Live default cron registry before/after the sandboxed complete cron-suite run: 106 jobs, identical IDs and byte-for-byte identical registry hash

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/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.5.1, Python 3.11

The full repository test suite was not run locally. The complete tests/cron suite was run and passed as documented above.

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A; behavior is internal and covered by code comments/tests
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — pure Python classification logic; no platform-specific behavior added
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

Not applicable. The change affects text classification in cron failure delivery summaries and is covered by focused and integration regression tests.

@angroni84-wq

Copy link
Copy Markdown
Author

@kshitijk4poor Could you please approve the fork workflow and review this focused cron bug fix when convenient? It distinguishes local pre-run script timeouts from provider failures. The isolated local cron suite passed: 678 tests, with the live cron registry unchanged during testing.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management labels Jul 10, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused cron fix. The premise is verified on current main: _run_job_script() emits the local envelope at cron/scheduler.py:2133, while _summarize_cron_failure_for_delivery() checks 429/rate-limit text first at cron/scheduler.py:63; failed jobs reach that classifier at cron/scheduler.py:3496.

The new precedence branch is narrow, preserves the existing provider classifications, and the PR covers both direct classifier behavior and the _run_job_script() integration path. Its diff context is unchanged on current main, so this should be mechanically salvageable.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 11, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing with credit — and a credit correction. The explicit "Script timed out after Ns" classification landed on main via PR #85536 (cherry-picked from #82460), but your PR was the EARLIEST submission of this exact fix — Jul 10, a month ahead. Our duplicate sweep missed it ("local script timeouts" phrasing); apologies for the misattributed first credit. The merged branch order matches your design: script contract before broad provider-keyword checks. Thanks!

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 P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants