Skip to content

fix(cron): surface external-worker dispatch failures through the incident path - #123432

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-123401
Closed

liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-123401

Conversation

@liuhao1024

Copy link
Copy Markdown

What does this PR do?

When the restart-safe external-worker handoff fails, run_one_job() recorded the failure on the job and in the executions ledger, then returned True before any operator-facing path ran: no cron_incidents row was opened and no failure notice left through the job's failure lane. A dispatch-level outage was therefore completely silent while executions.db accumulated status='failed' rows — exactly the situation reported in #123401, where a 9.5-hour outage (106 failed executions, including three jobs whose entire purpose is alerting) produced zero notifications.

This routes the dispatch-failure branch through _deliver_crash_failure(), the same machinery already used when run_job raises: it upserts the job+signature incident, delivers the failure summary through the job's failure lane, and marks the incident alerted once the notice is out. The existing alerted-cooldown semantics then apply, so repeated identical dispatch failures alert once instead of pinging per run. A notice-path exception cannot lose the bookkeeping: mark_job_run / finish_execution still run with a "failed" delivery outcome.

Related Issue

Fixes #123401

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • cron/scheduler.py — in run_one_job()'s handoff_error branch, call _deliver_crash_failure(job, error, adapters, loop) (wrapped defensively) before mark_job_run, and thread delivery_error / delivery_outcome into mark_job_run / finish_execution so the ledger records the notice outcome like the normal crash path does.
  • tests/cron/test_restart_safe_worker.py — add test_dispatch_failure_opens_incident_and_delivers_failure_notice: a failed handoff must open exactly one incident row (state alerted, original dispatch error), deliver one failure-lane notice, and finish the execution failed/delivered; a repeat of the same failure dedups to the same incident and is withheld (suppressed_acked, no second ping).

How to Test

  1. pytest tests/cron/test_restart_safe_worker.py -q — all pass, including the new regression test.
  2. Red check: with the cron/scheduler.py hunk stashed, the new test fails with assert 0 == 1 on incidents.list_incidents() — the silent-outage shape from the issue — and passes again with the fix.
  3. pytest tests/cron/test_oneshot_dispatch_failure_run_claim.py tests/cron/test_execution_ledger.py tests/cron/test_cron_failure_deliver.py tests/cron/test_cron_incidents_resolve.py tests/tools/test_cronjob_tools.py tests/tools/test_cronjob_run_background.py tests/tools/test_cronjob_run_delivery_notice.py tests/tools/test_cronjob_run_immediate.py tests/cron/test_scheduler_provider.py -q — all pass (Observed result: 223 passed across the dispatch/ledger/incident/delivery surface).
  4. ruff check cron/scheduler.py tests/cron/test_restart_safe_worker.py — 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/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 15 (arm64); the fixed path is platform-independent scheduler code

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

New regression test output (green):

tests/cron/test_restart_safe_worker.py::test_dispatch_failure_opens_incident_and_delivers_failure_notice PASSED

Red (scheduler hunk stashed) — the pre-fix silent shape from the issue:

>       assert len(rows) == 1
E       assert 0 == 1

…dent path

A failed restart-safe handoff in run_one_job() recorded the failure on the
job and in the executions ledger, then returned before any incident or
delivery path ran: no cron_incidents row, no failure-lane notice. Route the
dispatch-failure branch through _deliver_crash_failure() so it opens the
same job+signature incident and delivers the same failure notice as any
other job failure, with the existing alerted-cooldown withholding repeats.
A notice-path exception no longer loses the bookkeeping: mark_job_run and
finish_execution still run with a "failed" delivery outcome (NousResearch#123401).
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/cron Cron scheduler and job management sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Sep 26, 2026
@kiramakes

Copy link
Copy Markdown

🤖 Hermes Agent automated review: PR reviewed. Diff analyzed (94 lines changed). Check CI status and manual review recommended.

@kshitijk4poor

Copy link
Copy Markdown

Thanks @liuhao1024. This lands through #124013 with your commit and authorship kept. Two follow-ups were added on top: finish_execution still runs if the failure notice itself raises, and a failure after the worker has acknowledged the job only updates bookkeeping, labelled failed after handoff, with no dispatch-failed alert, since the worker may already have run the job. Your incident/cooldown test is kept.

@kshitijk4poor

Copy link
Copy Markdown

Merged: #124013 landed on main as ffe5cf0.

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 P1 High — major feature broken, no workaround sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Cron dispatch failures create no incident and send no failure notification

4 participants